Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

DOC: tweak reviewer guidelines. #11660

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jul 15, 2018
Merged

DOC: tweak reviewer guidelines. #11660

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
668f260
DOC: tweak reviewer guidelines.
tacaswell Jul 15, 2018
3466a47
DOC: tweak reviewer guide a bit more
tacaswell Jul 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 29 additions & 17 deletions 46 doc/devel/coding_guide.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,47 +41,59 @@ Documentation
PR Review guidelines
====================

* Be patient and `kind <https://youtu.be/tzFWz5fiVKU?t=49m30s>`__ with
contributors.

* If you have commit rights, then you are trusted to use them. Please
help review and merge PRs!

* For code changes (anything in ``src`` or ``lib``) two developers
(those with commit rights) should review all pull requests. If you
are the first to review a PR and approve of the changes use the
github `'approve review'
* Documentation and examples may be merged by the first reviewer. Use
the threshold "is this better than it was?" as the review criteria.

* For code changes (anything in ``src`` or ``lib``) at least two
developers (those with commit rights) should review all pull
requests. If you are the first to review a PR and approve of the
changes use the github `'approve review'
<https://help.github.com/articles/reviewing-changes-in-pull-requests/>`__
tool to mark it as such. If you are a subsequent reviewer and you
approve, either merge (and backport if needed) or select ``'approve
review'``.
tool to mark it as such. If you are a subsequent reviewer please
approve the review and if you think no more review is needed, merge
the PR.

Ensure that all API changes are documented in
:file:`doc/api/api_changes` and significant new features have and
entry in :file:`doc/user/whats_new`.

* Documentation and examples may be merged by the first reviewer. Use
the threshold "is this better than it was?" as the review criteria.

* Make sure the Travis, Appvyor, and codecov tests are passing before
merging.
* Make sure the Travis, Appvyor, circle, and codecov tests are passing
before merging.

- Whenever a pull request is created or updated, Travis and Appveyor
automatically runs the test suite on all versions of Python
supported by Matplotlib. The `tox` support in Matplotlib may be
Copy link
Member

Choose a reason for hiding this comment

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

Not sure what this phrase means. Why is tox support useful locally?

Overall, while you are looking at this, it'd be useful to have a short section explaining what all the tests do (and have folks update this as the tests change). The different Codecov tests are still a bit of a mystery to me, though I think I have a basic understanding of them. LGTM - who knows what thats testing, and if the current failures matter at all.

useful for testing locally.

* Do not self merge, except for 'small' patches to un-break the CI.
* Do not self merge, except for 'small' patches to un-break the CI or
when another reviewer explicitly allows it (ex, "Approve modulo CI
passing, may self merge when green")

* Squashing is case-by-case. The balance is between burden on the
contributor, keeping a relatively clean history, and keeping a
history usable for bisecting. The only time we are really strict
about it is to eliminate binary files (ex multiple test image
re-generations) and to remove upstream merges.

* Be patient with contributors.

* Do not let perfect be the enemy of the good, particularly for
documentation or example PRs. If you find yourself making many
small suggestions, either open a PR against the original branch or
merge the PR and then open a new PR against upstream.
small suggestions, either open a PR against the original branch,
push changes to the contributor branch, or merge the PR and then
Copy link
Member

Choose a reason for hiding this comment

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

Should the contributor consent before anybody pushes to his branch?

Copy link
Member Author

Choose a reason for hiding this comment

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

They have consented via the github UI (there is a little tick box off on the right). If they un-check that if they do not want to allow changes to be pushed.

Copy link
Member Author

Choose a reason for hiding this comment

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

but on a bit more consideration added a note about this. For small changes (spelling. style, etc) I think asking forgiveness is a better option, for big changes ask permission.

open a new PR against upstream.

* If you push to a contributor branch leave a comment explaining what
you did, ex "I took the liberty of pushing a small clean-up PR to
your branch, thanks for your work.". If you are going to make
substantial changes to the code or intent of the PR please check
with the contributor first.




Branches and Backports
Expand Down
Morty Proxy This is a proxified and sanitized view of the page, visit original site.