You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
@tacaswell
I am a newbie to matplotlib development and I was looking at low hanging fruits to pluck.
I tried my hand at fixing this issue and here is the basic changeset... sfroid@3b767f6
I have not added any tests or even done elaborate regression tests yet.
Before I do those, I wanted to know if
Should this fix be aimed towards the master branch or some other maintenance branch and merged back to master as well? I notice that the "Milestone" field on the right says v1.4.x. Is that the branch I should aim for?
What kind of tests should be written to protect this feature from failing again? Should we write an image based test or is a simple test which makes sure that the keyword is accepted without issues enough?
I am following (or going to follow) the development workflow for submitting this fix, but I am new to this, so please bear with me a bit.
That change looks reasonable. The next step is to open a Pull Request through the github interface (which is how we manage reviewing and applying). If you include #2899 (or #(any issue number)) in the text of the commit or anywhere on GH it will auto-magically link them.
Please target it at master. Typically new features go on master and bug fixes go on 1.Y.x, but we are in the process of getting the 1.4 release out so its a little messy. The 1.4.x milestone is what I have been tagging things that are not a priority to get done before 1.4.0, but should go on the 1.4.x branch when it exists. (Also to that end, if anything tagged an 1.4.0 and the label 'needs patch' seems interesting please work on those 😄 )
There some house keeping that needs to go with this change as well. You need to add an entry into the CHANGELOG (for devs) and whats_new.rst (for users), add linewidth to the docstring of pie, and probably re-generate the pyplot file.
As for tests, I think this should probably get an image test. I think that pie currently has an image test, use that as a template.
If you are feeling ambitious, my initial thought on this issue was actually to get pie to take **kwargs and pass them through correctly, but I am not sure what 'correctly' is here as we probably also want to be able to pass arguments through to the text.
I have a question regarding the feeling ambitious part, because I guess I am feeling so...
How about implementing it this way (see this commit sfroid@3ceade6)?
@tacaswell
I added a commit which needs review for the auto generated pyplot.py changes. sfroid@bd55ab2
Why is "linewidth=linewidth" added to the arguments of pie in pyplot.py (instead of wedgeprops)?
And why have all strings in function arguments in the pyplot.py changed from '' to u'' form? Is there something wrong with my particular setup which is making this happen?
Activity
sfroid commentedon Mar 22, 2014
@tacaswell
I am a newbie to matplotlib development and I was looking at low hanging fruits to pluck.
I tried my hand at fixing this issue and here is the basic changeset...
sfroid@3b767f6
I have not added any tests or even done elaborate regression tests yet.
Before I do those, I wanted to know if
I am following (or going to follow) the development workflow for submitting this fix, but I am new to this, so please bear with me a bit.
Thanks in advance.
sfroid
tacaswell commentedon Mar 22, 2014
@sfroid Great! We can always use more help.
That change looks reasonable. The next step is to open a Pull Request through the github interface (which is how we manage reviewing and applying). If you include #2899 (or #(any issue number)) in the text of the commit or anywhere on GH it will auto-magically link them.
Please target it at master. Typically new features go on master and bug fixes go on 1.Y.x, but we are in the process of getting the 1.4 release out so its a little messy. The 1.4.x milestone is what I have been tagging things that are not a priority to get done before 1.4.0, but should go on the 1.4.x branch when it exists. (Also to that end, if anything tagged an 1.4.0 and the label 'needs patch' seems interesting please work on those 😄 )
There some house keeping that needs to go with this change as well. You need to add an entry into the CHANGELOG (for devs) and whats_new.rst (for users), add
linewidthto the docstring ofpie, and probably re-generate the pyplot file.As for tests, I think this should probably get an image test. I think that pie currently has an image test, use that as a template.
If you are feeling ambitious, my initial thought on this issue was actually to get
pieto take**kwargsand pass them through correctly, but I am not sure what 'correctly' is here as we probably also want to be able to pass arguments through to the text.matplotlib#2899 Adding kwargs to pie which can handle args for both w…
sfroid commentedon Mar 22, 2014
@tacaswell
Thanks for the detailed reply.
I have a question regarding the feeling ambitious part, because I guess I am feeling so...
How about implementing it this way (see this commit sfroid@3ceade6)?
matplotlib#2899 almost final changes for adding wedge and text kwargs…
sfroid commentedon Mar 23, 2014
@tacaswell
I added a commit which needs review for the auto generated pyplot.py changes.
sfroid@bd55ab2
Why is "linewidth=linewidth" added to the arguments of pie in pyplot.py (instead of wedgeprops)?
And why have all strings in function arguments in the pyplot.py changed from '' to u'' form? Is there something wrong with my particular setup which is making this happen?
tacaswell commentedon Mar 24, 2014
You should create a PR, it makes this process easier 😉
tacaswell commentedon Apr 23, 2014
Closed by #2923