Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reorganize the developer docs #1512

Merged
merged 4 commits into from Nov 20, 2012
Merged

Conversation

mdboom
Copy link
Member

@mdboom mdboom commented Nov 16, 2012

This is a reorganization of the developer docs. The main thrust here is to add a "pull request checklist" which is intended to be a concise and easy-to-follow checklist of things to ensure before a pull request is merged. Some larger sections of coding_guide.rst have been separated out into their own chapters.

There are a lot of instances where we (myself included) have said "oops -- forgot to document this feature committed a while back" or "forgot to add a CHANGELOG entry" etc. I thought that having a place to refer to every time would be helpful, and should also help new contributors understand what's expected, and take some burden off of the seasoned developers to say "please add tests, documentation, etc." in the pull request review -- we now have something we can just point to.

I don't necessarily want to get into an elaborate discussion about coding standards -- and I've tried to keep this simple so as to not seem overly rigid and daunting -- but if there's anything that should go on this list that isn't there already, please mention it here.

@dmcdougall
Copy link
Member

This is exactly what I needed when I started contributing. Thank you so much for taking your time to put this together.


* Keep the maintenance branches and master in sync where it makes sense.
* No tabs (only space). No trailing whitespace.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be a main bullet, or a sub-bullet for the PEP8 bullet?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PEP8 doesn't address trailing whitespace. The "no tabs" rule is in PEP8, but I'd rather make it really prominent here.

@WeatherGod
Copy link
Member

Might want to add to the bullet list a reference to the section on what to do if adding a new plotting function.

from nose.tools import assert_equal

def test_simple():
'''very simple example test'''
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be a "proper" docstring, with """ instead of '''

@NelleV
Copy link
Member

NelleV commented Nov 19, 2012

I didn't take the time to checkout the code and building the documentation, but it seems to me like a huge improvement on the documentation. Nice job !

@pelson
Copy link
Member

pelson commented Nov 19, 2012

The main thrust here is to add a "pull request checklist" which is intended to be a concise and easy-to-follow checklist of things to ensure before a pull request is merged.

We should add a contributing.md file to help with that (for those submitting pull requests). Info: https://github.com/blog/1184-contributing-guidelines

@mdboom
Copy link
Member Author

mdboom commented Nov 19, 2012

@pelson: Cool: I didn't know about that feature of github. I agree, this content should go in there. But maybe contributing should just link to this page on matplotlib.org? I'm not sure keeping this documentation in sync in multiple places is a good idea.


Nose determines which functions are tests by searching for functions
beginning with "test" in their name.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A note in here would be useful regarding @cleanup.

@pelson
Copy link
Member

pelson commented Nov 19, 2012

Bravo! 👍 with the few suggestions I made.

Thanks @mdboom .

@mdboom
Copy link
Member Author

mdboom commented Nov 19, 2012

I (believe) I have addressed all of the comments above.

@pelson
Copy link
Member

pelson commented Nov 20, 2012

I (believe) I have addressed all of the comments above.

All looks good to me. Thanks @mdboom.

pelson added a commit that referenced this pull request Nov 20, 2012
Reorganization of the developer docs to make it easier to get up to speed with the matplotlib development process.
@pelson pelson merged commit 1fa3318 into matplotlib:v1.2.x Nov 20, 2012
@mdboom mdboom deleted the coding_guidelines branch August 7, 2014 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants