In practice, dealing with PRs of potentially questionable quality is still a lot of work. You have to review the code, test the changes, make sure the docs are up to date, and explain to the contributor in the nicest possible way where they messed up.
For some of my future projects, I'm even considering not accepting anything but docs and bugfix contributions at all due to the amount of work it takes.
There are many ways to solve any problem. If I reviewed every PR with the standard "is this exactly how I would have done it", I too would be destroying every PR I review. And I'm not even that good at coding - I'm the type of generalist who cares about things like the business beyond the code base.
What really matters is whether the code is solid and whether it will actually cause problems. Just because a PR isn't character-for-character how I'd write it doesn't mean it's bad. Based on the author's experience with the fired employee's replacement being just as bad, my guess is he was over defining goodness and raising the bar too high.
Bad PR to me means: bad runtime complexity, bad formatting, bad security, bad memory use, bad understandability. If it meets all those bars, which for a crud app is usually easy, then it should probably pass. I'll sometimes leave a "I would have done _x_ differently" comment on matters of taste _if I think it will help the other developer_.
A PR needs maintainer approval. And the maintainer I've seen thinks the documents are good enough already. In cases like that, a PR might not be able to solve the problem.
Complaining about it reroutes people to better projects, and pushes the project to fix the problem.
I wish this talked about the engineering time lost when a bad PR makes it to production. Fixing those may eliminate any time gains from skipping code review. Also, this violates security compliance standards (e.g. SOC2).
The problem I see with allowing PRs but not issues (or locking the reporting of them behind a paywall) would be that there is no way for someone to discuss possible implementation details without having already potentially have gone down the "wrong" path and wasted time (i.e. Maintainer won't accept the PR without a significant rewrite), or spent money to discuss it when they already plan to contribute time and code to solve their own use case.
One must also consider the typical quality of a PR - that is, lacking in many fundamental ways. Missing documentation, missing tests, doesn't match existing style, and sometimes just plain incorrect.
At some point, a flood of poor PRs can be worse than a flood of angry bug reports. The cost of validating and cleaning up a PR is significantly higher than closing a bug.
In my [limited] experience PR have always been orders of magnitude better [for me] than the average issue. Maybe it's because they focus on improving the project and not user's problems. It is true however that some times they were unsolicited and/or unwanted, however they were still a lot easier to process and review for me as a maintainer.
Of course if you remove issues and reroute people to PR the quality of them will lower (though not so low as Issues) while you will miss important things, but still seems like the net result is a easier to review _issues_ which might be what saturated maintainers want.
Edit: I totally agree with the last paragraph (and parts of others of course).
Yep, I've done these and #1 (all code is PR'd and reviewed) can be a right pain for technical and political reasons, and results in much slower progress and poorer overall code.
PRs are a wonderful mechanism to provide smooth but gated access to the code for people outside the owning team who otherwise would not have it at all; but inside the team, I think: just because you can do it does not mean that you should.
I've found PRs are usually a waste of time because people focus on such trivial bullshit because they're easy to find.
98% of the review comments I receive hark back to some handwavy explanation about maintainability. But never in any way that could actually cause a bug.
when I run teams my rules with PRs are if it's not a bug, and it's not against the code review guidelines leave it. It's usually not worth the back and forth unless there is a clear mentor/mentee relationship.
It doesn’t have to be like this. Even when I was CTO, I’d submit my code in a PR and my team would pull me up on mistakes and inconsistencies. It was really annoying! And also great.
PRs are a great way for the whole team to learn about how the organisation cuts code, and can reduce the number of errors, but of course with poor leadership they can be used for evil.
- Closing issues before they're resolved. Proper etiquette is to let the filer close it after it's fixed and working.
- People demanding features or making vague trouble reports.
- Submitting a properly-tested PR that fixes an issue without need for a fix-it issue.
- Committers open to some globally-beneficial change only to change their mind after it's already done.
- People refactoring your commit with something better.
- Rust and a lot of small languages have tiny, good communities of generally humble badasses.
- People changing whitespaces or switching to British grammar. SMH.
I look at it this way: you gotta take the shit with sugar. Not trying to contribute upstream is consuming without producing... I'm against this because it helps no one and it's uncool.
Sometimes I've had a pr merged. Sometimes I've had stubborn maintainers that think that their project should not follow common domain standards for /shrug reasons.
And then many other times I honestly don't have the knowledge to be able to contribute into large third party systems and it seems very rare to get an issue resolved without submitting a pr.
And lastly sometimes it's hard to judge if unexpected behavior is really a 'bug'. Similar to the last point, it can honestly be hard to tell the difference sometimes.
But "simple PRs" with poor implementations compound over time and make for outsized tech debt in the future. I agree that addressing specific major issues within the PR (e.g. comments) is not a good approach. If there are "enough" (for some relatively subjective definition of that) "teachable" things, it's likely an in-person (or at least chat thread) conversation is warranted. Under normal circumstances (and I'd hope a junior engineer isn't making a PR to patch something for an emergency/incident resolution), a PR should be fine to hold up, as the release timeline should account for the fact that junior engineers will have longer PR cycles.
1. Some people balk at you making lots of small PRs, because it feels like noise to them.
2. Some people/organizations make you do a lot of work on a separate branch and then want you to PR just once for the final feature you're implementing.
Yeah and I'm saying that's not a bad thing or even a rare occurrence. Sometimes PRs can be de-prioritized or forgotten for reasons that are outside of anyone's control, developers are human beings so that just happens. I've seen it in lots of open source projects.
I don’t think this is at all true... I mean the PR changes 350-odd files and adds some crazy JIT engine. It’s hardly low hanging fruit... I don’t know enough about how this is implemented to say it will make future features harder, they even state in the PR maintenance is very important to the design here so I’ll assume it won’t be a weight around their necks.
Ranging from "I wouldn't have implemented it that way" to "this opens up a huge security hole".
Security issues shouldn't be allowed in. But a lot of "maintainability" issues have negligible if any gains, and real costs like
it takes time to fix
could introduce new bugs
creates bad feelings between coworkers
reply