DOC: tweak reviewer guidelines. #11660
Conversation
tacaswell
commented
Jul 15, 2018
- include link to Brett Cannon talk
- re-order bullet points
- include link to Brett Cannon talk - re-order bullet points
doc/devel/coding_guide.rst
Outdated
| <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'``. | ||
| review'`` if you think further review is required. |
There was a problem hiding this comment.
I tend to approve explicitly also as a second reviewer, even when I merge immediately afterwards. IMO this makes it more clear who the two reviewers are. Since I'm usually on the git review page anyway, it's just two extra clicks - so not much work.
I don't demand, that everybody does it, but I would appreciate it.
There was a problem hiding this comment.
I like this, re-worded a bit.
| 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 |
There was a problem hiding this comment.
Should the contributor consent before anybody pushes to his branch?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
- encourage second review (even if followed by an immediate merge) - add note about etiquette for pushing to contributor branches
| * 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 |
There was a problem hiding this comment.
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.
…660-on-v2.2.2-doc Backport PR #11660 on branch v2.2.2-doc