Add an indicative limit for judging if a PR is too big in PR guidelines #6128
Conversation
pchila
added
documentation
pchila
requested review from
ycombinator,
michalpristas,
cmacknz,
blakerouse,
pkoutsovasilis,
kaanyalti,
andrzej-stencel,
michel-laterman and
swiatekm
|
This pull request does not have a backport label. Could you fix it @pchila? 🙏
|
|
|
mergify
bot
added
the
backport-8.x
|
I'm also adding folks from my team as we are working also on the Agent repository. |
pierrehilbert
requested review from
AndersonQ,
belimawr,
rdner,
mauri870,
faec,
leehinman,
khushijain21 and
VihasMakwana
|
The 500 line limit is the point where it is productive to suggest that a PR is too big, not an absolute limit that will be enforced without thought. This is codifying that it is explicitly OK for a reviewer to suggest splitting up a PR, which will hopefully lead to the reviewer and author collaborating on the best way to split the PR to make it easier to review. "I can't understand all this, please help me" by itself should be a fine review comment without even having a 500 line limit regardless. |
That is why I added the clause
to catch stuff like testdata in a unit test, generated code or anything else that does not require the same effort for review as code. For example a super complicated unit test of 2k lines of code for a single testcase may be deemed too complicated and too big (In that case the PR could be split by separating refactoring (if needed to simplify the testcases), tests and feature code in multiple PRs maybe) but in general single unit tests should be short and easy to understand The guideline is there to remind people that we should strive to keep the PR size small and that is ok to ask for a big change to be split in smaller ones and as @cmacknz said it's not a mindlessly automated hard limit... it will be evaluated case by case |
| @@ -44,6 +44,8 @@ Keep these in mind as both authors and reviewers of PRs: | |||
| - The PR must contain related changes and leave out unrelated changes (e.g. refactorings, etc. that could be their own PR instead). | |||
| - If the PR could be broken up into two or more PRs either "vertically" (by separating concerns logically) or horizontally (by sharding the PR into a series of PRs --- usually works well with mass refactoring or cleanup type PRs), it should. A set of such related PRs can be tracked and given context in a meta issue. | |||
|
|
|||
| * The PR changeset should be rather small: 500 lines of code (excluding generated code or other changes that normally should not require close review by a human) is the soft limit that should be used to judge if a PR is "too big". | |||
There was a problem hiding this comment.
LGTM 👍 500 lines is also used in OTel Collector guidelines:
We recommend that any PR (unless it is trivial) to be smaller than 500 lines (excluding go mod/sum changes) in order to help reviewers to do a thorough and reasonably fast reviews.
|
@kaanyalti I think we are only missing your approval here |
|
buildkite test this |
ycombinator
force-pushed
the
add-size-for-pull-requests
branch
from
5d8bf1d to
f7b342a
Compare
|
(cherry picked from commit 7d4ddfd)
@ycombinator: Let's wait till at least 2/3 of the reviewers tagged on this PR have approved it. It's an unusually high bar but I'd rather we clear that first than have pushback later.
What does this PR do?
Add indications about expected size of PRs in CONTRIBUTING.md
Why is it important?
To improve code review experience.
Checklist
[ ] My code follows the style guidelines of this project[ ] I have commented my code, particularly in hard-to-understand areas[ ] I have made corresponding changes to the documentation[ ] I have made corresponding change to the default configuration files[ ] I have added tests that prove my fix is effective or that my feature works[ ] I have added an entry in./changelog/fragmentsusing the changelog tool[ ] I have added an integration test or an E2E testDisruptive User Impact
How to test this PR locally
Related issues
Questions to ask yourself