Move fast and break things.
Merge vulnerabilities.
Double the work.
Merge code without tests.
Anything, but don’t let code become stale.
Probably unpopular opinion, but peer reviews are overrated. If coders are good AND know the project, the only thing you can do in a PR is nitpicking. They are more useful for open source collaborators because you want to double-check their code fits with the current architecture. But people here are reacting as if peer reviews could actually spot bugs that tests can’t catch. That happens rarely unless the contributor is junion/not good.
I operate from the presumption that code’s first job is to be as easy for a human to understand as possible. It should clearly communicate what it’s attempting to do. If your code isn’t written so that your colleagues, or you 2 years from now, can read it and understand it, it’s bad even if it’s whip tight, fits all the AC and has 100% test coverage with a perfect mutation score. That’s what I focus on when I review code: does it communicate intent semantically. Code that can be understood is code that can be reused, optimized, altered when use cases change, generalized out into even more reusable code, and provide insights that technically perfect but incomprehensible code can’t. I, like you, assume that the coder knows what they were trying to do and how to test for it, so that only gets a cursory glance to spot common errors like missed nullables, inverted conditionals and shit like that. I look at it from the perspective of “If I had to add functionality to this, could I do so easily”. Because I’m gonna one of these days.
Nitpicking can be automated by a linter, then reviews can actually sit back and review more important things like high-level design and scalability
as if peer reviews could actually spot bugs that tests can’t catch
There can’t be bugs if there are no tests to catch them! Ofc you can also automate test coverage standards. But PRs are sometimes the only way to catch bugs, even and especially with senior devs in my experience bc they are lazy and will skip writing tests, or write useless or bare minimum tests just to check off code standards and merge on ahead
Peer reviews can catch bugs that tests can’t catch.
I won’t disagree that peer reviews are overrated, but they’re a great way to train and onboard less experienced devs (who are just more fun to work with, anyway). Like I’m a platform dev, so I don’t have a “home” project - if I had to know every project before I opened a PR for it, I’d get hardly any work done. Review help other knowledge experts weigh in on my changes.
Anyway, one case for being pro
Nothing improves morale like the on-call having to unfuck production for the third time that hour because mUh VeLoCiTy decided code review and testing in CI was too slow.
Techbros are fucking cultists.
The subtle Linux-flex in the screenshot.
I kind of with the sentiment. Review pre merge though, but only block the merge if there are serious faults. Otherwise, merge the code and have the author address issues after the merge. Get the value to production
Get the value to production
Ugh, not this SAFe Agile ™ cultist bullshit. The “value” is working, bug free code, which you get when you put it through review and QA before it gets to production.
There’s often features and bug fixes worth more than the ones introduced in the PR. I’ve yet to see bug free code just because it’s went through review and QA.
Surely you’ve seen bugs caught because code went through review and QA though. Those are bugs that would go into production if following the “advice” in this post.
I’m with you. I’ve worked on a few teams, one of the first was a company where two teams were contributing code changes to the same product. The other team “owned” it and as a result it took ages, sometimes months, to get code changes merged. It meant more time was spent just rebasing (because merging wasn’t “clean”) than working on the actual feature.
My current role, we just do TDD, pair programming, and trunk-based development. We have a release process that involves manual testing before live deployment. Features that aren’t ready for live are turned off by feature flags. It’s quick and efficient.
Fundamentally I think the issue is the right tool for the job. Code doesn’t need to be managed the same way in a company as it does in an open-source project.
Code doesn’t need to be managed the same way in a company as it does in an open-source project.
Open-source projects are usually longer lived more maintainable, more stable, and more useful than any closed source code I’ve ever worked on for a company. Partially because they’re not under contract deadlines which create pressure to “deliver value” by a certain date, but still. Might be helpful for companies to consider adopting practices the open-source community has shown to work, rather than inventing their own.
have the author address issues after the merge.
Hahahahahahaha. Sorry, you’ve merged, next ticket, PM needs shiny results for execs this QBR!
This is how bug backlogs grow.
This is some poe’s law shit. I can’t tell if you’re serious or just committing to the bit.
Sorry about the confusion. It’s not sarcasm. I’m just sick and tired of people blocking my PR because of an argument about wether the function should be called X or Y or Z or D
Ah. Yeah those kind of nitpicks are annoying. We try to specify when comments are blocking or non blocking on reviews.
But I definitely block a lot of reviews over no tests, bad tests, no error handling, failed linting. And the occasional “this doesn’t do what the ticket asked for”
This only works if the merge is being done to staging builds that are continuously tested by a QA team before they go to production, with carefully planned production milestone releases. I work for an emergency management SaaS company. If we just merged all lightly reviewed code into production without thorough QA testing, there’s the possibility that our software would fail in production. This could cause aircraft in major airports to crash into each other on the runway, or a university to respond poorly to a live shooter situation, or the deletion of customer data about COVID vaccine efforts, etc
Test it? Meh. Just ship it.
