[dead]
PR process killing morale and productivity
221–224 of 224 posts
Re: PR process killing morale and productivity
#222My team has a bike shedding sort of problem where a 100 loc PR will sometimes get scrutinized to hell, but a 3,000 loc PR will get LGTM'd by enough of the team to be merged before anyone that actually cares gets a chance to look at it. I would say the second half of that is the much bigger problem. People know who to ask to get a quick lgtm. I don't know what to do about it. I can't make people actually review. I've…
Speaks volumes to the maturity of our work that a 3000 LOC PR is seen not only as acceptable, but expected. Raising a planning change equivalent to a 3000 LOC PR in a civil engineering firm would get your assignment swiftly handed over to someone more competent.
But the planning change to add a new wing to the building? That can’t be made smaller. What about if a new regulation is put in effect requiring every fire door to have a corresponding fire window next to it? That’s either 3000 little changes that need reviewing or one big change. One big change is often the correct choice. Knowing when it’s appropriate and when it’s not is the job of the engineer, the team, and the lead.
Re: PR process killing morale and productivity
#223Earlier quoted context omitted.
IMO, the best way is to have automatic formatter shared by team.
Isn’t that a given in 2024? I find it amazing that it still seems to be an issue in some places. Using a linter/formatter is a no-brainer at this point, and it’s been for years.
There's probaly a ton of projects that don't even have version control in 2024, let alone a linter.
Re: PR process killing morale and productivity
#224Earlier quoted context omitted.
> Not having the braces makes it so easy to accidentally break in later changes or refactoring. Did you mean “not having a linter”? Because once you have a linter you no longer need the braces because the autoformatting will always fix the indentation.
I was thinking of a case like this: if (foo) return; Perfectly valid and people do it. But then later on someone might put a debug statement above the return to check something. Or add some other logic. if (foo) doSomething(); return; And suddenly the logic is broken and the return falls out of the if-condition. It looks fairly contrived with an example like this but easy to do when code is a bit more complicated or…
I think you misread what I wrote. Your example could be fixed by braces, but it can equally well be fixed by just automatically fixing the incorrect indentation. Visual Studio has been doing this for a decade already. It's really not rocket science.