docs: code review guidelines and tips #4532
Conversation
Signed-off-by: Larry Gritz <[email protected]>
| - Aim for small PRs that do a specific thing in a clear way. It is better to | ||
| implement a large change with a series of PRs that each take a small, | ||
| obvious step forward, than to have one huge PR that makes changes so | ||
| extensive that nobody quite knows how to evaluate it. |
There was a problem hiding this comment.
Might be worth mentioning that PRs should link to any other PRs that need to land first?
There was a problem hiding this comment.
I was more thinking of the case of the same person having subsequent PRs queued up but not yet submitted, waiting for earlier work to be accepted.
There was a problem hiding this comment.
Ah that's fair. I've done it both ways when trying to have dependent features submitted.
| examination of the code make them say "yes, I see, of course, great, just as | ||
| I expected!" | ||
| - Make sure your PR includes a test (if not covered by existing tests). | ||
| - Make sure your PR fully passes our CI tests. |
There was a problem hiding this comment.
Might also be good to suggest it passes any linters that your project requires too?
There was a problem hiding this comment.
CI doesn't pass unless all that stuff works, so it's redundant in this project. Even not having correct formatting fails a CI test.
|
|
||
| Not all patches need the highest level of scrutiny. Reviews can be cursory and | ||
| quick, and merges performed without additional delay, in some common low-risk | ||
| circumstances: |
There was a problem hiding this comment.
I might also suggest that in these scenarios , it's worth doing the following:
-
leave a message on the PR as to why you're merging without review.
-
broadcast that you did so in the community (slack)
Those just allow someone to historically review things without blocking it.
There was a problem hiding this comment.
Yes, good suggestion, thanks.
| * PRs languishing for weeks or months without any review or merge. | ||
| * PRs that don't adequately test their changes (basically crossing your | ||
| fingers and hoping it works). | ||
| * PRs that are merged despite failing CI that might be related or, if |
There was a problem hiding this comment.
One more that I might suggest adding is something along the lines of avoid unilateral rejection of ideas.
Obviously this would be discretionary as there are some ideas that would never pass muster (e.g rewrite the project in a new language) or the submitter cannot defend their viewpoint beyond basic scrutiny.
However, in the case where the submitter is able/willing to defend their suggestion, it would be recommended to bring in a second reviewer or bring it to the TSC.
There was a problem hiding this comment.
Thanks, I will amend. A "rejection" should always be one step in a conversation, with the next step being the author either fixing the patch, trying to explain/justify it as-is, or realizing themselves that a different approach is needed and closing their own PR. I don't believe a reviewer should ever close a PR, unless it's clearly been abandoned by the author, or unless it utterly fails to conform to the minimum standards of any submission (i.e. based on form, not simply disagreeing with the content).
| GitHub, or by adding a comment that says "LGTM" (looks good to me). | ||
| - If possible, reviewers should have some familiarity with the code being | ||
| changed (though for a large code base, this is not always possible). | ||
| - At least a few work days should elapse between posting the PR and merging |
There was a problem hiding this comment.
Might also be worth adding something like "submitters should be given some time (a day?) to review changes made to their PR by a reviewer, in case they have useful context/thoughts/objections to add"
There was a problem hiding this comment.
In general, if I make suggestions in the form of an explicit diff, I expect authors to be the one to click the "accept" button. It feels rude to push something to their branch, unless they ask me to or it's clearly the only way to move the PR forward. But yeah, I agree that even if the reviewer has to push a change, they should give the author a chance to okay it before a merge in almost all circumstances. I will say something to that effect.
Signed-off-by: Larry Gritz <[email protected]>
|
Thanks for the thoughtful comments, @dgovil. I pushed an update with some amendments. I think this is a PR that I'll leave up for quite a while, as I'd love to have plenty of time for people to digest this and make additional suggestions. |
|
If there are no further comments forthcoming, I will merge this over the weekend (which will have given it a full 2 weeks of comment period). We can always continue to modify both the document and the policy goals over time if needed. |
No description provided.