Reduce number of open PRs to 2 or 3 per dev

[JohannesLorenz]

The large number of PRs is often being criticized, and it has a lot of disadvantages:

1. Merge conflicts and even that two developers work on the same thing are hard to predict (like e.g. the mentioned between Fix resize issues #7524 and Feature: Detach window #3532). No one can follow 7 pages of PRs.
2. Similarly, old PRs keep getting merge conflicts.
3. It's discouraging for developers that their PRs are not being reviewed.

I think all developers should be forced to keep at most 2 or 3 PRs open (1 may not be enough if you find out you need 1 or 2 more PRs to get an open PR working - e.g. I assume #7554 was used to help #7459 ). All other PRs have different options:

1. If they are just incomplete, but not damaging anything, they can be merged to master. An example would be CI: Run LMMS #7427 which works (worked?) perfectly on Mac and Linux CI, but I was waiting for Dom's help for Windows for more than one year. I will merge that, soon - Windows can be added later.
2. If they were not continued for years, we might close such PRs, the branches are pushed to something new like lmms/lmms-archives and a PR draft can be opened there.
3. If they just await review, they should be tagged accordingly.

Suggested rules for new PRs:

1. We reject new PRs completely until we reach our goal of 2 or 3 PRs per dev
2. We reject new PRs if at least one of our PRs exists for more than 3 months

That way, everyone who would have opened new PRs now is forced to review existing PRs.

[CorruptVoidSoul]

I am not a dev, but I approve of this idea.

[firewall1110]

Suggested rules can be interpreted as a barrier for new developers, but not be scared and if You (new developer) will have problem to open new PR:
(1) write your protest (even here);
(2) wait until "this storm ended".

So I suggest some exceptions (make some PRs as exceptional - not count neither in "2 or 3 PRs per dev", nor in "exists for more than 3 months" ):
(1) for old PRs working for new features (example #5990 );
(2) for new bugfix PRs.

No one can follow 7 pages of PRs.

And no one can follow 40 pages of Issues ...
Solution (one of ...) : - make consolidation meta issues (and such for PRs should be renewed to stay on 1-st or 2-nd page). But it is hard work now ...

Also I suggest some rules for new PRs:
(1) No PR without Issue and work start (and ideas, plan, ...) should be "marked" in comments.
(If Issues are not duplicated this prevents "two developers work on the same thing" ).
(2) Rules in Contributing Step by Step must be followed
(this prevents incompleeted PRs)

Brand new features, such as Audio Recording , External Synchronization with jack transport , should be planed in small steps (on Isuue level) and work should be started only after LMMS member approving this plan (than new Issue can be opened for every small step , then implemented, PR created, reviewed, improved, approved and merged to master). (But old ones should be continued as started - as exception.)
This prevent total chaos , that make, for example, #5990 staled for years.

P.S.
But it is only my opinion - members knows better how to manage LMMS project.

[tresf]

I'm having a hard time understanding this rule. I think it aims to have some type of "you scratch my back, I scratch yours" enforcement but I'm having a hard time understanding the execution of it all.

I don't want to minimize the merge-conflict-mess that large PRs cause, but I'm reading that there should be an arbitrary count of how many PRs can be open per developer, which I'm failing to understand completely.

I believe we should more aggressively be converting stale PRs to drafts to shorten the list. (Similar to your archive recommendation)

I also believe that coordination (such as you working today with @messmerd on common PR class nomenclature and design) in regards to overlapping code, concepts and ideas is paramount.

However, my fear is that such a rule would ultimately punish (reject) good work in defence of a policy. Perhaps put better - a developer that writes a lot of small patches shouldn't be bottlenecked in the same fashion as a developer who makes a sweeping change through 300 files, or one that completely refractors or overhauls a commonly used component.

[JohannesLorenz]

I believe we should more aggressively be converting stale PRs to drafts to shorten the list. (Similar to your archive recommendation)

I think stale PRs should rather be archived, because otherwise, they will just enhance the list of PRs, making the list - again - too big to overview. Now, you could filter those out (which many people will not do by default), but even then: I often use "draft" for if I want to show a PR that will soon go into review - and these should show up. So IMO there are 3 categories:

1. Active developed PRs -> Should stay normal
2. PRs that are preparing for their first review -> Should be draft
3. PRs that are stale for a long time -> Should be archived IMO

[tresf]

I think stale PRs should rather be archived, because otherwise, they will just enhance the list of PRs,

I think this statement is generally true.

[softrabbit]

More or less ex developer here, but anyhow...
(I don't know when (if ever) I'll do much LMMS development again, so feel free to disregard the ramblings of an old fart)

I think @tresf is closest to the truth here. 10 small bugfixes is not too much for a single developer, but one huge "Let's change X just for X's sake" refactor might be. (I'd much rather see stuff like that in a separate commit in a PR where you actually do something - fix a bug, improve a feature - unless it is something that must change everywhere at once. You know, a little touch up while you're in the neighborhood anyway.). And besides, the larger the PR, the more conflicts with other open PRs it creates on merge.

So, it's really not useful to put down a hard limit on the number of PRs, but maybe on the number of files or lines they touch?

I think taking care of stale PRs would be a lot better, and that would include preventing them by dealing with new PRs in a timely manner. It's not like anyone likes updating a PR to get around some arbitrary cutoff rule, and then get the thumbs down a long time later. Granted, it's been a while since my last ones, so I don't know how long it takes to get your PR reviewed these days (and I don't mean purely in a formatting sense), but I can imagine the devs doing reviews being swamped. I know my familiarity with the code never was at a level where I'd be comfortable telling contributors how to do what they do, and I'd expect a pretty small minority to have the needed amount of hours to put in to really get there. So that would be a bit of a problem (unless I'm just pessimistic).

I'm not too sure on breaking new features into steps in separate issues and having a PR for each. Maybe for something huge, but one issue with a checklist of the steps should be enough most of the time, instead of filling the issue list. The PRs could then implement parts of that list in suitable chunks.

But hey, sometimes more regulation and procedure is the solution.

Inspired by this discussion, I had a look and closed my old stale PRs. What I had was:

• One (mostly) refactor that was IMO an improvement, but as other changes had stomped all over it (~every line in a huge file was conflicted) in the years it gathered dust I decided euthanasia was the right way to go. Too bad about the hours I poured into it, but I'm not going to add more to that number for something that mostly just prettifies working code.
• One that was a bugfix, but it should've probably been a larger overhaul (or so I'm told) instead of just fixing the issue. The issue is of course still open for anyone to fix.
• One that was a beginning of a feature, but I ended up waiting for the reorganising of 2021(?) to blow over and lost momentum (and I mean LMMS in general). Now that PR was probably way too conflicted to be worth it.

[JohannesLorenz]

So, it's really not useful to put down a hard limit on the number of PRs, but maybe on the number of files or lines they touch?

This might sound more fair at first, but it also will increase the number of PRs, i.e. the number of topics a developer must keep in mind to avoid conflicts. Then, other PRs soon end up on page 2 of the PR overview and no one sees them anymore.

My point is that if a developer has smaller PRs, these have a higher chance of getting reviewed and their review is usually also much more faster - so smaller PRs might be blocking, too, but not for long. So this IMO compensates the "it's unfair because some devs have smaller PRs".

[regulus79]

I feel like one of the main things determining whether a PR gets reviewed and merged isn't necessarily the size, but just how often you talk about it on the discord lol.
I have multiple small, quality of life PRs which only touch 10-50 lines and are (hopefully) ready to merge, but since I haven't been talking about them often, people haven't been testing/reviewing them very much.

[tresf]

@regulus79 these PRs should get the attention they deserve. Feel free to keep plugging them on Discord. Thanks for the patches and to the dedicated testers helping find bugs. You've been so very responsive to all the testing that's been occurring. Until we shrink the PR list, it's definitely a squeaky wheel syndrome.

[tresf]

That way, everyone who would have opened new PRs now is forced to review existing PRs.

I mean, I guuuueeees we could create something like the "golden rule" tag for PRs if we're worried that authors are programming in a way that's a bit too selfish/narcissistic for the project.

Note

🟨 Golden Rule:
"Do unto others as you would have them do unto you"

Reviews for this PR are discouraged for the following reason:
Author has not recently contributed feedback to other open PRs.

... of course, this can quickly become a cat-and-mouse game. e.g.

• Two golden rule violators are discouraged from reviewing each other's PRs (yuck)
• May encourage review-for-the-sake-of it rather than good reviews (yuck)
• Review teaming (🤮 )

... not to mention the backlog of PRs that were opened before such a quota existed.
... not to mention the additional overhead by us to manage (or automate) all of this.

@JohannesLorenz I want to be clear... I'm not against encouraging better cooperation here, but due to the small amount of active developers, it may be easier to just call these individuals out as we see fit.

[JohannesLorenz]

Another way to solve this:

We could write a script which first uses a REST query to check all PRs. Then, it would clone all the respective branches and then could tell you for every file who is recently working on it.

Disadvantages of this are

1. You will need to check this regularly, such as using a git diff against the git merge-base master, and display for all those files who is currently working on them. So, if you just suddenly start writing on a file and forget to call that script, the script has no effect.
2. In unlikely cases, two people can work on the same thing but in different files.