21–30 of 76 posts
Pull requests are really awesome. Slowing certain things down on purpose can help you go a lot faster.
I strongly disagree with this article. I wouldn't trust myself or my my coworkers to produce perfect code every time. We have 2 reviewers mandatory for each PR and using pull requests is a perfectly acceptable way to ensure quality - every line of code has to be vetted by at least 3 developers (it has to pass the CI/CD pipeline checks as well). If you produce high end software with complex workflows and/or calculatio…
> 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".
It's written to trigger me right? I have introduced prs on low quality teams and guess what happened? Higher quality, better learning, faster development because we do not constantly fix bugs in production. Of course automate as much as possible, make the pr cycle short and explain people that creating the pr is not at 99% and you only wait for the button press but more like 70% and expect learning and small rework a…
> 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…
> 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 out the window and it takes weeks to recover (unless you just lock out everyone until shit’s been fixed). It’s already hard enough when you just have flaky code, or time-sensitive tests.
If you want light reviews that’s a factor of development & review policy: let people self-review, make review scope clear, break up major changes into small tasks, …
With that said, despite advocating at points for pair programming, mobbing, incremental reviews and having worked on a variety of different systems, and having success with some of them, I keep ending up back with PR's for the majority of places I've worked.
The reason why, I think, is one of cognitive load. Pull requests are easy. I don't think they're the best approach to code review ( big PR's often don't get reviewed effectively, and being an effective PR reviewer is a hard skill ) but, it's a good fallback. Everyone knows what a pull request is, the tooling for supporting them is excellent and it requires the least upfront work to get a code review.
With that said, I would highly encourage folk to look at different ways of reviewing code. I don't think many people will fully replace Pull Requests with a solely straight to master development approach utilising non-pr code review styles, but, having the alternatives in your box of tools for specific situations is really powerful.
However, I don't see pull requests going away because, if nothing else, they're an easy, low cognitive load approach for having a second set of eyes on some code.
For 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.
With junior devs is too easy to become something in which the senior dominates the conversation.
This is very nearly touched on in the "pipeline approvals" section:
"...you place the gate after code integration and automated tests. Doing this means that a human only spends time reviewing code that has already been proven technically correct."
That's a very good use case for a pull request with CI that builds/tests the result of the merge.
Is 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.
Where I am, and at other good companies that I know of, "a change being accepted into main" equals processes being automatically started, that progress it through further environments and test suites, that if they all go well, result in "it becoming live in production" with no further human intervention.
So you oversimplified, but it's a qualified yes.
Continuous deployment is a very safe and productive way of working. It encourages small batches of change and thorough automated testing. If you find it "ludicrous" that says more about you than anything else. There are books on the subject if you're interested.
Is 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.