sort large PRs

[casswedson]

Summary

None

Purpose of change

this was a feature request on a comment on the recent upcoming review guide, based on one of the checklist items for PRs: try to avoid large PRs >300 loc of cpp source files and header files >1000 loc of json

• use git diff to get the amount of changes in a PR, note this is deletions + additions

• label pr that pass the loc limit with a label needs review and leave a comment courtesy of Erk https://discord.com/channels/598523535169945603/598535827169083403/1217569735571537961

this is a draft because I want feedback on these two things, like yes/no to labeling of just comment on the PR by the way it does not block anything, it says "Merging has been blocked" but it's just a bluff

one limitation this has is that it won't warn if the PR gets larger over time because that's a little more complicated, and I fear it would take quite some time to figure out

Describe the solution

Describe alternatives you've considered

Testing

https://github.com/casswedson/Cataclysm-DDA/actions/runs/8288186147/job/22682229734?pr=144 running on https://github.com/casswedson/Cataclysm-DDA/pull/144 logic is good https://github.com/casswedson/Cataclysm-DDA/actions/runs/8288186147/job/22682229734?pr=144#step:3:1 github is being github again, and I can't quite test the labeling step cause it's not letting me access resources for whatever reason https://github.com/casswedson/Cataclysm-DDA/actions/runs/8288186147/job/22682229734?pr=144#step:4:1 other than that this baby is good to go

Additional context

[Qrox]

use git diff to get the amount of changes in a PR

Maybe add the ignore whitespace option? As noted in previous discussion some PRs may simply change the indentation of code and result in a large number of changed lines.

[casswedson]

add the ignore whitespace option?

yes will do

some PRs may simply change the indentation of code

you mean like, someone makes a PR without using the formatters, and it appears like they changed a lot? or just large indentation changes, that sounds like something you should avoid doing in the first place

to me a PR with a large amount of json or cpp changes either messed up somehow or is rather big. giving said PR a needs review would be reasonable

[Inglonias]

Any PR that does not conform to our style guide is already blocked from being merged. I don't think we need to worry about indentation changes causing false positives.

I also think that if we don't have the authority to block the merge, we shouldn't say that we do.

[Qrox]

you mean like, someone makes a PR without using the formatters, and it appears like they changed a lot? or just large indentation changes, that sounds like something you should avoid doing in the first place

For example, if you add an if condition around a large code block it will change the indentation of the entire code block, and in that case adding the ignore whitespace option will make it only count the two lines of the if statement instead.

[PatrikLundell]

github is incompetent and creates artificial "differences":

• Indentation: Changing the logic so a loop is no longer needed causes the indentation to change for every line in the loop. Has happened to me. If you find a loop construct that can iterate over a block rather than nesting X and Y you remove an inner loop, for instance. You also get the same effect if you create a block in order to ensure a constant is local to that block. Again, all the lines within that block gets indented.
• Failure to identify what's changed can cause the tool to think a lot of changes have occurred simply because it cannot sync up the actual change correctly. If you add a block with a structure similar to a set of existing blocks it can get the idea that you changed the differing component of each of these blocks, for instance.

Also, I think the label logic is backward even if you were to use competent tools for comparisons: Instead of labeling every small PR I would rather mark the bulky ones indicating extra work may be needed to review it. That aligns with the pattern of adding labels to indicate additional stuff is affected (such as e.g. mods), rather than the pattern of a long string of tags indicating passed tests.

[kevingranade]

Not something that will block merging this is if other stuff is sorted out first, but the message it prints should be more of a policy statement and call to action. Basically something like, "this is a large PR, please review the (link to PR size guidelines). Unless it falls under one of the exceptions listed there, you will need to split or otherwise reduce the size of this PR."

[casswedson]

I just realized the guide is still under review; I'll wait a little until it's finished to continue

[Inglonias]

#72374 is merged. This PR can be undraftified.

[Brambor]

the message it prints should be more of a policy statement and call to action.

Completely agree.

I don't see why are labels needed. The comment (call to action) is very good, as it says the person how they should improve. Are the labels for reviewers? Will they use them? For what? Do they want to filter out large PRs? Or filter them to tell the people that they should split it?

The only way I see is that some reviewer doesn't want to look at anything big right now, so they pre emptively filter out anything big. Still, 300 can be much more complex than 150. If they could filter like 10 lines changed, I could see them use that.

I ramble too much about this, sorry.

[casswedson]

Well this is awkward, I am going to be away from my setup for a about a week; I would be glad to hand down this PR if someone is willing to pick up where I left off, what's left to refine is all in the hands of the maintainers regardless. :)

[Inglonias]

I'm afraid I'm going to have to insist that we come back to this one. It's just too important that this gets resolved before it happens again.

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. Please do not bump or comment on this issue unless you are actively working on it. Stale issues, and stale issues that are closed are still considered.