Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
ENH Improving the contribution guidelines#7236
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
hi@choldgraf |
A recent build of the documentation is here:http://cbio.mines-paristech.fr/~nvaroquaux/tmp/matplotlib/devel/contributing.html Any feedback is welcomed. |
| 2. Fork the `project repository | ||
| <https://github.com/matplotlib/matplotlib>`__: click on the 'Fork' button | ||
| near the top of the page. This creates a copy of the code under your | ||
| account scikit-learnon the GitHub server. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
scikit-learn reference left.
| import matplotlib.collections as mcol | ||
| import matplotlib.patches as mpatches | ||
| * If your changes are non-trivial, please make an entry in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We do not maintain the CHANGELOG anymore
| :file:`CHANGELOG`. | ||
| * If your change is a major new feature, add an entry to | ||
| :file:`doc/users/whats_new.rst`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ask to create a file indoc/user/what_new (and see the README in that folder. This greatly reduces merge conflicts. The files are all merged periodically.
| * If your change is a major new feature, add an entry to | ||
| :file:`doc/users/whats_new.rst`. | ||
| * If you change the API in a backward-incompatible way, please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ditto for putting in thedoc/api/api_changes folder.
| document it in :file:`doc/api/api_changes.rst`. | ||
| * Adding a new pyplot function involves generating code. See |
tacaswellOct 11, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
lets not extend the pyplot inteface....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍
| * In general, simple bugfixes that are unlikely to introduce new bugs | ||
| of their own should be merged onto the maintenance branch. New | ||
| features, or anything that changes the API, should be made against | ||
| master. The rules are fuzzy here -- when in doubt, try to get some |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would say 'when in doubt, target master'
I am 50/50 on renaming the rst files. The new names are better, but they are going to breaksomebodys bookmark. |
| @@ -12,14 +12,18 @@ The Matplotlib Developers' Guide | |||
| .. toctree:: | |||
| :maxdepth: 2 | |||
| coding_guide.rst | |||
| portable_code.rst | |||
| license.rst | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would leave this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Are you talking about the license.rst? It doesn't really fit in the developers section. There is currently a link to it somewhere in the user documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, but if it is still linked to via a visible link, then ignore my comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, it is
@tacaswell good point about the bookmarks. I hadn't thought about this. I'll revert back to the old names. |
I've addressed@tacaswell's comment (a refresher was clearer a good idea!) I still have to add the new contributor tag. |
I've added a section about the new-contibutor-friendly tag and the easy-fix tag and made some changes to the style. I am still not convinced that the new-contributor-friendly tag adds more to the table than the easy-fix one. we'll have to see long run whether it is worth having both. A fresh build of the documentation is available here: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Comments and corrections are mostly minor.
| In case you experience issues using this package, do not hesitate to submit a | ||
| ticket to the | ||
| `Bug Tracker <https://github.com/matplotlib/matplotlib/issues>`_. You are | ||
| also welcome to post feature requests or pull requests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is it worth adding something about the matplotlib-users mailing list, encouraging its use for "how do I do..." and "why does matplotlib do this" questions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Especially if we continue with "experience issues", then it's even more prudent to mention it.
| Submitting a bug report | ||
| ======================= | ||
| In case you experience issues using this package, do not hesitate to submit a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
"In case you experience issues" -> "If you find a bug in the code or documentation"
| After obtaining a local copy of the matpotlib source code (:ref:`set-up-fork`), | ||
| navigate to the matplotlib directory and run the following in the shell: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I like to make sure I don't have any version of matplotlib already installed in whatever location I am about to install the development version. Is it worth making a recommendation like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, it is definitely worth it.
I think it might be interesting to have a FAQ of common encountered problems with setting up working environments with matplotlib. This is one of the problem my students encountered, and the other problem being the tests massively failing.
| pip install -v -e . | ||
| This installs matplotlib for development (i.e., builds everything and places the | ||
| symbolic links back to the source code). You can then run the tests your work |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
"tests your work" -> "tests to check that your work"
| symbolic links back to the source code). You can then run the tests your work | ||
| environment is set up properly:: | ||
| python tests.py |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If this is run at this stage it will spew image comparison failures, won't it? Maybe better to delay all mention of running the test until the place below where you tell how to set things up for testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
On my computer, it works fine at this stage. I am lucky?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's not strictly problematic, but if you don't tell it to use the fixed version of freetype, you might run into a lot of image failures if all the text has shifted a bit (like say, the next versions of Debian and Fedora, which include a newer FreeType.)
| code. The interface code should be named `FOO_wrap.cpp` or | ||
| `FOO_wrapper.cpp`. | ||
| * Header file documentation (aka docstrings) should be in Numpydoc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't understand this bullet point at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
eh… me neither…
I could just remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Presumably this is about docstrings that live inside C/C++ code... Likethisresample docstring. I would just change it to "Docstrings should be in Numpydoc..." - since this is already under theC/C++ extensions header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I am in favor of leaving this section as is (but maybe burying it a but further down the page).
| Matplotlib makes extensive use of ``**kwargs`` for pass-through | ||
| customizations from one function to another. A typical example is in | ||
| :func:`matplotlib.pylab.text`. The definition of the pylab text |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
pylab -> pyplot
| i.e., it just passes all ``args`` and ``kwargs`` on to | ||
| :meth:`matplotlib.text.Text.__init__`:: | ||
| # in axes.py |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
axes/_axes.py
| local arguments and the rest are passed on as | ||
| :meth:`~matplotlib.lines.Line2D` keyword arguments:: | ||
| # in axes.py |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
axes/_axes.py
| :file:`matplotlibrc` (:ref:`customizing-matplotlib`) supports an | ||
| external backend via the ``module`` directive. if | ||
| :file:`my_backend.py` is a matplotlib backend in your | ||
| :envvar:`PYTHONPATH`, you can set use it on one of several ways |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
delete "use"? or delete "set"?
All of the following looks like it might be replaced with a link to somewhere else in the docs.
On 2016/10/12 8:12 AM, Nelle Varoquaux wrote:
I think it means you just happen to have the right version of freetype |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There were a few more comments than I expected; I might just commit some parts directly if you're fine with it.
| @@ -357,6 +357,19 @@ div.admonition, div.warning { | |||
| font-size: 0.9em; | |||
| } | |||
| div.warning { | |||
| color: #b94a48; | |||
| background-color: #F3E5E5; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The added colours are in varying case.
| margin-top: 10px; | ||
| padding: 7px; | ||
| border-radius: 4px; | ||
| -moz-border-radius: 4px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Did you write this yourself, or is it generated somehow? Why only add the-moz prefix? It seems likeborder-radius was standardized a long long time ago.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's from the nature stylesheet of sphinx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ah, I see. I guess they just haven't updated them in a while.
| Adding new scales and projections to matplotlib | ||
| *********************************************** | ||
| ======================================================== | ||
| Developer's guide for creating scales and transformation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
transformations
| ************ | ||
| Coding guide | ||
| ************ | ||
| ******************** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Reviewer's
Also, I think there's one extra asterisk here.
| In case you experience issues using this package, do not hesitate to submit a | ||
| ticket to the | ||
| `Bug Tracker <https://github.com/matplotlib/matplotlib/issues>`_. You are | ||
| also welcome to post feature requests or pull requests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Especially if we continue with "experience issues", then it's even more prudent to mention it.
| Note: there is a use case when ``kwargs`` are meant to be used locally | ||
| in the function (not passed on), but you still need the ``**kwargs`` | ||
| idiom. That is when you want to use ``*args`` to allow variable | ||
| numbers of non-keyword args. In this case, python will not allow you |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is that common? It seems like something that'smostly an artifact of Matlab's designs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
And we mirrored that API very closely at the beginning. There is probably a few more cases (but I do not know any off the top of my head).
This should have a comment added that this is only a python2 restriction, as soon as we drop 2.7 as supported we can use keyword-only arguments 😄 (that is the python3 feature I like the most).
| ---------------- | ||
| We have hundreds of examples in subdirectories of | ||
| :file:`matplotlib/examples`, and these are automatically generated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
"these" is ambiguous; it sounds like theexamples are automatically generated, but it's really their outputs that are automatically generated.
| ==================== | ||
| ========================= | ||
| GitDevelopment workflow | ||
| ========================= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks like an extra=?
| .. _portable_code: | ||
| ===================================================== | ||
| Developer's tips for writing code for python 2 and 3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Python
| @@ -17,20 +18,16 @@ Requirements | |||
| The following software is required to run the tests: | |||
| - nose_, version 1.0 or later | |||
| - `mock <http://www.voidspace.org.uk/python/mock/>`_, when running python | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
See above comments, but these links might be outdated.
dopplershift left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
edited Oh, I see now I was only seeing the latest set of changes and commenting on them.
| .. _nose: https://nose.readthedocs.org/en/latest/ | ||
| .. _pep8: https://pep8.readthedocs.org/en/latest/ |
dopplershiftOct 13, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Since you're updating these URLs, would this be a good time to move to readthedocs.io? (Seehere)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What do you mean by this? Move the matplotlib documentation to readthedocs.io?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
oh… I just understood :)
| @@ -296,7 +288,7 @@ Keyword argument processing | |||
| Matplotlib makes extensive use of ``**kwargs`` for pass-through | |||
| customizations from one function to another. A typical example is in | |||
| :func:`matplotlib.pylab.text`. The definition of the pylab text | |||
| :func:`matplotlib.pyplot.text`. The definition of the pylab text | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Would it make sense to changepylab topyplot in the text as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
yep… I actually think that we may want to do a git grep pylab on the whole project and check individually whether to move it to pyplot. It's on my todo list.
| @@ -10,17 +10,17 @@ customizations to the nose testing infrastructure are in | |||
| :mod:`matplotlib.testing`. (There is other old testing cruft around, | |||
| please ignore it while we consolidate our testing to these locations.) | |||
| .. _nose:http://nose.readthedocs.org/en/latest/ | |||
| .. _nose:https://nose.readthedocs.org/en/latest/ | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Again about RTD.
@dopplershift I |
| @@ -26,7 +26,7 @@ | |||
| par1.set_ylabel("Temperature") | |||
| par2.set_ylabel("Velocity") | |||
| p1, =host.plot([0, 1, 2], [0, 1, 2], label="Density") | |||
| p1, =par1.plot([0, 1, 2], [0, 1, 2], label="Density") | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Was that supposed to change in this commit? (It's the rtd.org -> rtd.io commit.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
that was a mistake. I'll revert it.
Sweet, thanks for going ahead with that! |
I think this is ready to be merged. |
@ivanov was interested in giving feedback on this pull request. |
👍 - I wasn't sure why readthedocs links were changed from .org to point to .io, so I looked it up and foundthis explanation (in case anyone else ends up wondering about that the way I did) |
Is there anything else to be done on this PR for it to be merged? |
* DOC adressed reviewer's comments* DOC rdt.org -> rdt.io
FWIW, I'm trying to get up-to-speed on how to contribute to matplotlib, and this PR would be helpful for somebody in my position. It looks like this is close to being ready to go, looking forward to seeing some updated docs |
There's one failure on Travis, which is unrelated, since this PR does not introduce any code changes. (for reference - the failure's I'm going to go ahead and merge this one, and we can iterate further. Good work everyone, especially@NelleV ! Happy hacking! |
🍻 |
ENH Improving the contribution guidelines
ivanov commentedNov 2, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
backported to |
(oops, had the wrong SHA in github comment above, fixed it now - it's28f4b6e ) |
Thanks@ivanov ! And welcome back :) |
Uh oh!
There was an error while loading.Please reload this page.
Here is a first attempt at improving our documentation on how to contribute. It is greatly inspired from the scikit-learn. I tried to merge all documents into one main document for new contributors.
What is left to be done
Open questions
An overview of the current documentation can be seen here:http://cbio.mines-paristech.fr/~nvaroquaux/tmp/matplotlib/devel/contributing.html
closes#7234