Couple of other good articles about async code review linked here: https://news.ycombinator.com/item?id=36029328
Why your team doesn't need to use pull requests
41–50 of 76 posts
Re: Why your team doesn't need to use pull requests
#42Is the author suggesting every commit goes into the main branch, and into production? That seems ludicrous. The trick with merge requests is to keep them as small as possible while being something that can potentially be deployed into production, ie some kind of fix or feature, not just dead code.
Does a change being accepted into master equal it becoming live in production at your company? That seems even more ludicrous.
As others have said, not everything going into main goes straight to production, but it's the start of the process that should see it deployed.
Nothing should go there unless it's a candidate for deployment.
To me continuous deployment means deploying features as they are ready (and the smaller the better), rather than batching up big 'releases'. It does not mean deploying code line-by-line.
All IMHO of course.
Re: Why your team doesn't need to use pull requests
#43Can we all just agree that the author is incorrect. We do not need to be second guessing fundamentals like code review and keeping mainline branch working. The goal of making software is to make working, profitable software. This person has lost perspective.
Re: Why your team doesn't need to use pull requests
#44> “As they work on it” is essential. As a team member, you don’t wait until you have finished a feature or story to integrate your code to the mainline. Instead, you frequently - at least once a day - put your code into a healthy state that passes tests and integrate it into the mainline with everyone else’s current work.
Maybe I work on very different code bases to the author, but having half-done work merged into `master` is a terrible idea. The sands are always shifting under your feet as a developer and it's almost impossible to successfully integrate manual testing into your process (testers need a stable base to test against). Yes, you could feature-switch, but it's easy to make a mess of things and it nullifies some of the "integrate often" benefits.
> No form of code review is more effective than pairing. Feedback is immediate, so there is a far higher chance you will use it to make improvements.
So pull requests are too costly, but pairing isn't? Pairing may reduce one class of errors, but it likely exacerbates others. If the reviewer is "too close" to the code being reviewed they'll miss many of the same things that a single developer would. Not to mention, it's exhausting for a lot of people (myself included).
PRs aren't magic, but there's a reason they've become so popular. And no, it's not just management liking "control" or something - they make the codebase better, which makes the developer experience better.
Re: Why your team doesn't need to use pull requests
#45[deleted]
Re: Why your team doesn't need to use pull requests
#46> Using pull requests for code changes by your own team members is like having your family members go through an airport security checkpoint to enter your home. It’s a costly solution to a different problem. No, it's like having an editor proofread your article before you submit it. It's a second pair of eyes to make sure you haven't missed something or made a change that will subtly break things, or just misundersto…
Also as far as I can tell OP’s recommendation is to break the mainline, then tell everyone it’s broken. > Instead, you frequently - at least once a day - put your code into a healthy state that passes tests and integrate it into the mainline IME that’s a perfect recipe for getting a mainline which never passes tests in any organisation of a non-trivial size, as soon as broken code has been pushed “giving a fuck” goes…
100%
The author also seems to have missed that you can still use CI on feature branches. Merge `master` to your feature branch regular, run the CI. Sure, you're only integrating with other complete-ish work, but it gets you a long way without making a complete mess of things.
Re: Why your team doesn't need to use pull requests
#47I fail to see the real world benefit of this approach.
Replacing pull requests with pair programming doesn’t make adding features to the main line necessarily faster. Review still happens, just synchronously, and you have two people working full time on the same thing, instead of one.
Personally, I have never worked in an environment where a few hours delay in reviewing a pull request, was such a big of a deal.
It also seems to disregard the fact that continuous testing can and does happen, because a feature branch may run against all the usual test fixtures, e.g. plan branches in Bamboo work for this.
Re: Why your team doesn't need to use pull requests
#48Earlier quoted context omitted.
> We have 2 reviewers mandatory for each PR > every line of code has to be vetted by at least 3 developers Those are not equivalent. Reviewers typically don‘t "vet every line of code".
2 reviewers seems like overkill unless it’s they’re also testing actual working. Reading code in a PR is not worth so many people-hours.
It depends on what is required from the reviewers.
If you have a pair with 1 person with good knowdledge of the code digging deep into what the PR/MR does, and another dev just checking for anything surprising, or even just validating that what you're doing makes sense at the surface, that will probably cover most bases.
Re: Why your team doesn't need to use pull requests
#49For us this wouldn't work. PR's are a great way of giving feedback and teaching junior devs. It also gives people people an idea of whats going on with the codebase and provides a way to align code style. Pair programming.. sure.. but PP is so resource intensive and often it's better to have people focus by themselves imho.
> Pair programming.. sure.. but PP is so resource intensive and often it's better to have people focus by themselves imho. Wholeheartedly disagree on the last part. Yes, Pair programming is expensive, but the knowledge transfer is invaluable. I would encourage people to work together on the same problem.
I've done pairing in the past, and as much as it was fun and felt productive, it was completely exhausting. I don't know how people do it day-in-day-out.
I always wonder if the pairing advocates and extroverts, and if that's why it hasn't really caught on in a field with a high percentage of introverts.
Re: Why your team doesn't need to use pull requests
#50Earlier quoted context omitted.
> We have 2 reviewers mandatory for each PR > every line of code has to be vetted by at least 3 developers Those are not equivalent. Reviewers typically don‘t "vet every line of code".
2 reviewers seems like overkill unless it’s they’re also testing actual working. Reading code in a PR is not worth so many people-hours.
Unless you work in a very complex technical domain with high profit margins. A bug in production can literally cost us billions (apart from potential reputation damage), the system is highly complex, so having 2 reviewers isn't a luxury for us but a necessity.