[policy, no patch] relax patch size policies

On commit of the issue that originated this issue catch made the following comment 📌 Replace REQUEST_TIME in services Fixed which supports the existing policy

I'm going to go ahead and commit this despite #66 because I've already reviewed it in its current state once and it's looking good now. However the re-roll hell this has been in since December validates that it probably would have been easier in smaller chunks - e.g. splitting cache changes out to their own issue, or core/modules changes or both. It's always hard to balance between too many changes in one issue vs. too many issues for one change, and can be hard to tell when you start working on an issue how big the eventual MR is going to end up, so very tricky to figure out.

Where, the comment being referred to, comment #66, is the one where it was asked that the issue be split into smaller issues.

Being forced to split a large MR up just because it touches more than X number of files/lines/KB is just busy work to me, especially with these types of issues where the changes are mostly the same thing across every file.

On the other hand the policy is based on research that shows that the quality of a code review reduces as the change size increases. I do think Drupal should follow that, unless there is new research. And I know from my own experience as a reviewer that the research is correct.

Or is it more that this policy isn't a hard rule and we can decide on an issue-by-issue basis as a community whether something is too large?

There have been exceptions to the policy about the size of the change, such as when it is mostly boilerplate or a large change was automated. I think one of the recent OO hook issues was one, but I could be wrong. Coding standard issues would be another case where large patches were committed. So, yes there is some flexibility.

The language of the policy is already flexible in that it takes in consideration of the type of change and only suggests a 'good target size'. I do not see a need for a change.

Yes, hook-to-OOP went in a single go. However, that contained no functional changes and while the MR itself was over 25 000 lines it was just shuffling code around automatically and a very small amount manually and even that small amount was split into six patches. Zero functional changes. (The problems being found are coming from the new module system not the conversion. People have abused ModuleHandler in truly creative ways I couldn't have possibly expect.)

I think it would be good to consider maybe upping? I really get both sides

📌 Move helpers in contact_storage_test.module and delete it Active this is a piece of a meta converting all test modules to hooks. The fact it was broken out to this smaller one made it super quick to review. If the developer tried to group say 6 modules of tests to convert that could take a little longer. And this may be a bad example but could hold up good to go pieces from getting in.

So I'd be +1 for upping it slightly but also recognizing and suggesting that breaking things up is always the fastest approach.

I think it's funny that issues I created were used for pro and against increasing the patch size.

I think it should be increased a bit from where it is, and while it can get hard to review after a certain size, tools like hiding sections do make it easier.

Here is a third example.
I am working through an issue that it was requested I break up now too.

📌 Mark core modules as converted to OOP hooks Active
📌 [pp-1] Mark several more modules as converted Active
I think the first break up makes sense because the first issue was an attempt to mitigate the memory leak and the issue was really doing two separate things to that approach.

The second issue is being requested to be broken up into 3 new issues.

With the tools in gitlab it feels possible to review that as one issue especially since it was rtbc already in the parent issue.
I'll likely break it up when I get some time just because it was requested, but I think that issue should be considered acceptable.

As for the meta smustgrave mentioned I did combine a couple but quickly realized the remaining had too much to do in each so I will end up splitting them up as I work on them as well.

For those that want this to change, can you add suggested wording to the issue summary so it can be evaluated? Showing before and after text would help.

Do we just want to increase the number and leave the language?

A good target size for a merge request or merge request is a diff size 10-40K, or about 400 changed lines of code. The ideal merge request size varies with the type of fix.

Good conceptual scope is more important than the size alone. Large size merge requests with many instances of the same simple change can still be easy to create and review, while small merge requests with complex code or documentation can be difficult to create and review.

These limits are based on 11 Best Practices for Peer Code Review. That analysis shows that reviewers are twice as likely to miss problems when reviewing more than about 400 lines of code, and many times more likely to miss problems when reviewing more than 400 lines.

Thought?

I think 400 lines seems reasonable, I think the main change being that we're not judging by patch "size" since that metric is not readily available from an MR (AFAIK).

I wonder if we could find some examples of >400 line MRs that are considered too hard to review.

sometimes it's easier to chunk commits by 400 loc but having overall changes as 10k