Patch proposal (extension module)
Hello, I would like to contribute to Inkscape, and particularly to the scripting interface. I would like to submit my first patch: - various typos corrections - simplify awkward if statement - move Script::file_listener method definitions in source file
Raphaël
Hi Raphaël,
welcome aboard! Hope you enjoy contributing to Inkscape. Thanks for your E-mail and contribution.
On Sat, 12 Mar 2016 14:50:15 +0100 Raphaël Bournhonesque <raphael0202@...48...> wrote:
Hello, I would like to contribute to Inkscape, and particularly to the scripting interface. I would like to submit my first patch:
- various typos corrections
- simplify awkward if statement
- move Script::file_listener method definitions in source file
Your patch seems like it does several welcome changes, but one problem I see is that it is not an atomic and self-contained change. In general every commit/changeset should do one thing and do it well, and with being as unintrusive as possible (and without breaking the automated tests or introducing new bugs). Your patch does too many things.
Perhaps consider submitting individual and atomic changes as patches to the launchpad bugtracker.
Regards,
Shlomi Fish
Hello, Thank you for your remarks. I'm not really familiar with bazaar, as i've mostly used git. "git add" accepts a really useful --patch command that allow to add part of a file to the index. Do you know if bazaar offers similar options ? It would be really helpful to generate atomic patches.
Best, Raphaël
On 12/03/2016 18:52, Shlomi Fish wrote:
Hi Raphaël,
welcome aboard! Hope you enjoy contributing to Inkscape. Thanks for your E-mail and contribution.
On Sat, 12 Mar 2016 14:50:15 +0100 Raphaël Bournhonesque <raphael0202@...48...> wrote:
Hello, I would like to contribute to Inkscape, and particularly to the scripting interface. I would like to submit my first patch:
- various typos corrections
- simplify awkward if statement
- move Script::file_listener method definitions in source file
Your patch seems like it does several welcome changes, but one problem I see is that it is not an atomic and self-contained change. In general every commit/changeset should do one thing and do it well, and with being as unintrusive as possible (and without breaking the automated tests or introducing new bugs). Your patch does too many things.
Perhaps consider submitting individual and atomic changes as patches to the launchpad bugtracker.
Regards,
Shlomi Fish
On Sat, 2016-03-12 at 21:05 +0100, Raphaël Bournhonesque wrote:
Hello, Thank you for your remarks. I'm not really familiar with bazaar, as i've mostly used git. "git add" accepts a really useful --patch command that allow to add part of a file to the index. Do you know if bazaar offers similar options ? It would be really helpful to generate atomic patches.
I would be interested to hear if you can use git-bzr plugin:
https://inkscape.org/en/develop/using-git-repositories/
It might be an option for you depending on your operating system.
Inkscape will be moving to git in the future, it'll take some time though so we need to use bzr for now.
Best Regards, Martin Owens
Thanks, I will give it a try. By the way the debian package mentionned on the site is now obsolete, it should be git-bzr-ng instead.
On 12/03/2016 21:26, Martin Owens wrote:
On Sat, 2016-03-12 at 21:05 +0100, Raphaël Bournhonesque wrote:
Hello, Thank you for your remarks. I'm not really familiar with bazaar, as i've mostly used git. "git add" accepts a really useful --patch command that allow to add part of a file to the index. Do you know if bazaar offers similar options ? It would be really helpful to generate atomic patches.
I would be interested to hear if you can use git-bzr plugin:
https://inkscape.org/en/develop/using-git-repositories/
It might be an option for you depending on your operating system.
Inkscape will be moving to git in the future, it'll take some time though so we need to use bzr for now.
Best Regards, Martin Owens
I attach the first patch (cosmetics, and tab replacement by 4 spaces) to this email. It would be much easier for me to submit new patches one this one is accepted.
Sincerely Raphaël
On Mar 12, 2016 12:06, "Raphaël Bournhonesque" <raphael0202@...48...> wrote:
Hello, Thank you for your remarks. I'm not really familiar with bazaar, as i've mostly used git. "git add" accepts a really useful --patch command that allow to add part of a file to the index. Do you know if bazaar offers similar options ? It would be really helpful to generate atomic patches.
Late reply, but in Bazaar you can use "bzr shelve" to save part of your changes for later.
Best regards, Krzysztof
participants (4)
-
Krzysztof Kosiński -
Martin Owens -
Raphaël Bournhonesque -
Shlomi Fish