Docs: Add PR review guide - #24051
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #24051 +/- ##
==========================================
- Coverage 80.99% 80.98% -0.01%
==========================================
Files 1106 1106
Lines 383330 383330
Branches 383330 383330
==========================================
- Hits 310467 310456 -11
- Misses 54545 54554 +9
- Partials 18318 18320 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
365a836 to
6760aad
Compare
github-actions
Bot
added
documentation
|
|
||
| 1. Is the feature or fix covered sufficiently with tests (see the [Testing](testing.md) section)? | ||
| 2. Is the code clear, and fits the style of the existing codebase? | ||
| See the [Reviewing Pull Requests](pr_review.md) guide for what we look for |
There was a problem hiding this comment.
I moved and expanded the content into its own page
6760aad to
7aec50c
Compare
github-actions
Bot
removed
the
development-process
2010YOUY01
left a comment
2010YOUY01
left a comment
There was a problem hiding this comment.
Thank you. I read through it and strongly agree!
asolimando
left a comment
asolimando
left a comment
There was a problem hiding this comment.
The PR review guide reads well, I personally learned a few things along the way already (I didn't know "ablation testing" practice had a name), left some minor/optional comments
Co-authored-by: Yongting You <2010youy01@gmail.com>
Co-authored-by: Yongting You <2010youy01@gmail.com>
…nto alamb/pr_review_guide
There was a problem hiding this comment.
Thank you @alamb This will be very helpful for everyone.
| 1. Concisely describes the **problem being solved from the user's point of | ||
| view**. |
There was a problem hiding this comment.
+1 to this, I've lately seen some unnecessary long PR descriptions with a sloppy vibe, so whatever we can do for doubling down on this, that'd be great.
Sloppiness tends to make its way not only to the PR description, but also to the Github comments in response to feedback. That would fall more on a PR author guide though, not necessarily this one.
(no action item in this comment)
|
|
||
| 4. Explicitly calls out any user-facing or API changes (see | ||
| [Review the Code](#review-the-code) below). | ||
|
|
There was a problem hiding this comment.
Another one that comes to mind:
5. Doesn't dwell on implementation details or explain what the code does.
That can help keeping PRs concise and leave implementation detail explanations to comments in the code instead. The point being that people should not focus on re-explaining implementation details in the description that are already implicit in the code and comments.
There was a problem hiding this comment.
I do find it useful if the PR description points out the key code changes so that, as a reviewer, you get some context and starting points to know where to start your read through of the code. I agree that you don't want to repeat/describe the entire implementation of course.
Reminds me of https://gruhn.me/blog/2026-08-03/. You want to have a high signal-to-noise ratio description of the implementation changes; not a verbose explanation that repeats technical minutiae but doesn't add much information.
There was a problem hiding this comment.
I think all agree here, but since it's a matter of degree of details, it's somewhat subjective.
Keeping in mind that the PR description also becomes the commit message, which IMO is more useful as higher-level than lower-level, one can also use other means to provide more details on the implementation for reviewers:
- follow-up comments in GitHub with an implementation walkthrough that wouldn't read nicely in a commit message
- self-review comments on specific line(s) of code
Not sure it's worth going into so many details in the PR review guide, but it might be an option if the proposed line finally needs to be expanded
There was a problem hiding this comment.
A bit more concretely, the PR template asks "What changes are included in this PR?". What do we expect people to answer here and at what level of detail? The template says "a summary of the individual changes in this PR". I always interpreted that as the key changes to the code that were made.
There was a problem hiding this comment.
I think it's fine to write a line for each non-trivial change in a function/file there, but it should still read high level, no code snippets/quotes, something like: "filter.rs#function_xyz: clamping the number of distinct values with the total number of rows" instead of citing the formula or corner cases, focusing more on the "what" than the "how". I think this is what @gabotechs means by "dwelling" in his sentence.
When looking at git history, in general, I am looking for the "what", then I can use --name-only to list all affected files, and git show to check the code itself, but anything that can spare me those two extra steps in an investigation is gold.
Not sure how other feels about this, especially as I haven't been in the community for too long to voice the general feeling, just sharing my own personal perspective and what I saw in other projects, so take it with some grain of salt :)
There was a problem hiding this comment.
Keeping in mind that the PR description also becomes the commit message, which IMO is more useful as higher-level than lower-level, one can also use other means to provide more details on the implementation for reviewers:
I agree commit message should be a concise 'what' summary, however I think making the PR description as the commit message is not a good default configuration, I'll try to find is there any better settings, like adding a 'summary' section in PR template, and only use its content as the commit message.
A bit more concretely, the PR template asks "What changes are included in this PR?". What do we expect people to answer here and at what level of detail? The template says "a summary of the individual changes in this PR". I always interpreted that as the key changes to the code that were made.
My personal habbit is
- try to explain most rationale in code comment
- In PR description only provide tldr and pointers (e.g. 'please first read code comment at
file Aandstruct B, then follow along to understand the entire PR')
This way there will be no hidden info that only exist in PR writeup, and people reading related code from somewhere else won't miss anything; also PR description can further help reviewers to understand.
There was a problem hiding this comment.
I tried to summarize this conversation in commit 4be3186
I don't think I really captured @2010YOUY01 's suggestion though (which is great) -- this feels more like "best practice" advice rather than something to check off on a pr_review.
Maybe we should add it to the template? Or make a separate "best practices" doc area
|
thankyou for all the feedback -- I hope to incorporate this feedback tomorrow |
|
Ok, I think i have addressed all the feedback on this PR. I'll plan to merge it tomorow -- and we can always improve on this in follow on PRs |
|
Thank you everyone for your feedback |
9 participants
Which issue does this PR close?
Rationale for this change
As our project grows both in terms of number of users as well as the number of PRs submitted (due to agents and increasing usage) I would like to trying to document / automate as much as possible
As one of the largest bottlenecks at the moment is PR review, so making that more efficient I think will help us improve the flow of code in the project and make best use of our committers' time. My rationale is that by documenting this process more clearly
I also strongly believe effective documentation should be written for both humans and agents so I purposely didn't make a specific skill for this (instead I made a skill that points at the relevant parts of the docs)
What changes are included in this PR?
Are these changes tested?
By CI
Are there any user-facing changes?
New doc page