doc: change line threshold and reword subsystem test in large PR policy - #65685
doc: change line threshold and reword subsystem test in large PR policy#65685Renegade334 wants to merge 1 commit into
Conversation
Signed-off-by: Renegade334 <contact.9a5d6388@renegade334.me.uk>
|
Review requested:
|
|
For real-world context, this is a list of all of the commits in the last two years that would have triggered the line threshold after this change, but would not have done so before: Details9cf2ce7 deps: update perfetto to 57.2 Most of these are automated dependency updates that would not qualify anyway. However, the three human PRs in this list are prime examples of what should really be considered a large PR: |
Proposes two material changes to the large PR policy:
Firstly, the current line calculation (
abs(added - deleted)) only really covers PRs that add or remove large featuresets. Changes to existing lines are calculated as +1/-1 by Git, so a PR that edits a million lines in-place has a "net size" of 0 and doesn't count as a large PR by this definition. A PR that makes 5,000 changes to existing code is no less challenging or time-consuming to review than one that adds 5,000 of new code, and we should include these under the policy.This is a change from the approach discussed in #62829, so I'm anticipating that this will generate some discussion. However, the case example provided (splitting a flaky test into several subtests) would already be excluded under this policy, and 5,000 is no small threshold; it's easy to miss (or hide) something in a massive PR that purports to be doing something mundane.
Secondly, the policy wording states "add a new subsystem", but really intends to state "add a new public module"; the given example of adding a
node:foo/barmodule would not create a new core subsystem (it would come underfoo) but should still be considered a large PR. Altered the definition accordingly.I've also moved the paragraph that clarifies the /deps count to underneath the paragraph that it elaborates on, for greater readability.