> 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 misunderstood the requirements and implemented something not quite right, or didn't implement your tests properly and thus are not actually testing what you think you are.
CI isn't some magical fairy that ensures program correctness; it's only as good as the tests it runs. And even the best tests do not guarantee correctness.
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 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, …
Yes. I work on a monorepo with >1000 other devs, in every time zone. If we just broke mainline all the time, you'd get people merging broken code in Asia at the end of their work day and then we would have to revert it in NA. That would be a nightmare!
Also I wish our CI took 10 min :( As of now it can take up to 90 minutes just to compile the codebase (although caching usually mean it's much faster than that.)
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.
Some CI systems do this automatically for PR builds, e.g. TravisCI:
> Rather than build the commits that have been pushed to the branch the pull request is from, we build the merge between the source branch and the upstream branch.
No. You and those replying to you have completely misunderstood what the author wrote. My emphasis and a clarifying adverb:
> 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 [then] integrate it into the mainline with everyone else’s current work.
You get your local copy into a healthy state - build passing, tidied up, refactored - and then you push it upstream. The idea is precisely that you only push healthy code, and so never break mainline.
No, we understand perfectly well what the author wrote, but we’ve also worked with actual breathing humans rather than just spherical cows in vacuum, so we understand what will inevitably happen.
This is about as realistic as telling devs not to push bugs. As soon as it gets in the way or there’s a boss breathing down the dev’s neck, anything which is not enforced is jettisoned.
As far as i can tell from your comment ("as far as I can tell OP’s recommendation is to break the mainline"), you did not in fact understand what the author wrote.
I've worked in trunk-based development shops for fifteen years. My colleagues have mostly been good, but certainly not superhuman. It works fine.
> As far as i can tell from your comment you did not in fact understand what the author wrote.
I like how you keep calling me a liar, it’s nice.
> I've worked in trunk-based development shops for fifteen years. My colleagues have mostly been good, but certainly not superhuman. It works fine.
The “E” in IME stands for Experience.
I’ve worked in trunk-based development shops for years as well, it did not work fine, my comments are experience from seeing it break down as the number of devs increased, and the time wasted on mainline being broken kept ticking up.
I’m happy for you if you never suffered from that, but I’m not going to argue for practices I’ve seen fail over and over.
So basically everything that one would normally finish with a PR, but without a PR.
In my daily work, I never reject a PR for failing tests (the CI system disallows merging until all tests, lint, warnings, danger, style have passed). So whenever I find a problem in a PR, it's due to something that is not detected (or possibly not even detectable) by the automated checks. By dropping the PR requirement, you lose that added check.
FWIW in a team of only experienced (>10 years) devs, I find problems in approximately 20% of the PRs I review (granted our codebase has numerous subtleties due to the esoteric nature of our domain). We've also had problems that even the reviewer missed; problems that broke mainline and made merging impossible until the issues were fixed.
It's fine to say "we'll just accept the added risk", but you really need to do it with eyes open. I personally won't accept such risks unless I absolutely have to (i.e. can't find a reviewer and the customer is breathing down our necks). That's one thing that 25+ years in the industry has taught me.
Yes, but they are unconvincing. Pair programming is only acceptable to some developers (others it drives insane). It also induces tunnel vision because now both developers are too close to the code to see issues that an outside observer will pick up quickly. They're also intimately familiar with the structure and algorithms, and so they won't see things that someone fresh would have trouble grasping (meaning that they need to either simplify, name with intent, or comment appropriately). I actually haven't noticed too much of a difference in issues picked up on pair programmed vs solo programmed PRs; they tend to match the quality of the stronger developer.
I've done periodic code reviews at three separate companies. Unless I've been very unlucky, it generally goes like this:
* Everyone gets pulled away from their work for an hour and goes into a room.
* Someone has picked some code to review (either they've been forced into it, or they have something cool they want to show off). Mostly they're just terrified at the prospect of being humiliated in front of everyone.
* Everyone pretends to be interested, but they're really just waiting for it all to end as the CO2 levels slowly climb.
* A few minor details are talked about, and then someone asks why approach Y wasn't used. Relieved that there's finally a discussion point, everyone talks about this architectural idea for the rest of the hour.
* Due to time constraints, 90% of the code is never reviewed.
I spend about an hour a day on average reviewing PRs, and that's with a team of 5 engineers. If we reviewed all code in team meetings, that would multiply review time by 5x because now everyone is reviewing all code in series rather than in parallel.
Then there's this:
> Pipeline approvals: If your team uses a Continuous Delivery pipeline to deliver changes to production, you can include a stage that requires someone to authorize the change to progress. This is conceptually similar to a pull request in that it is a gate in the delivery process, but 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.
Except it hasn't been proven technically correct. It's just code that has been (maybe) pair programmed, and (maybe) discussed if it happened to be part of the 10% of the code looked at and discussed for architecture in the review meeting.
Yes, and this is how things were supposed to go before PRs. Whenever PR requirements are added, it's always because the described process does not work in reality and mainline ends up filling with crap as developers under pressure try to rush their prio-1 features out the door.
> This practice has become so common that many people consider it a default, “best” practice. Some people assume there is no other way to make sure code is reviewed because they’ve never seen anything else.
And you can see this in the comments on this post: a few are "we did trunk based development and it didn't work, so we switched to pull requests", but most are "we do pull requests, and i can't see how trunk-based development could possibly work", so not reporting a switch based on experience.
And (as you may have seen me mention, sorry), i have been doing TBD for years, and have mostly not had a problem with the trunk filling up with crap.
Exactly, but it's also important to note that different teams have different requirements, and the QA on those will be different for all.
Pull requests with code reviews obviously make since in a large number of environments. But so does committing directly to a single dev branch for others.
> CI isn't some magical fairy that ensures program correctness
So very true, it much better to see it as a check for regressions or a small number of very well defined core areas. Proper user testing by someone who is trying to break it is so much more important.
Far too often small teams try to use tools and procedures that have been designed for large teams and businesses, when in reality they have been designed to solve organisational issues that small teams don't have.
The article actually touches on other methods of review later on.
I agree with a lot of points the author makes: there are other methods of increasing velocity and reviewing code that don't involve costly PRs. But they've become doctrine at many companies, even tiny startups that don't need a lot of process breaking velocity. Know the right time and context.
His alternatives to PRs are pair programming and after the fact reviews (when the code is already in the mainline). I don't see any orgs where we would have wanted that, small or big team.
I think the main disconnect is that, decreasing velocity is the point of a PR in a way. We usually want to stop the dev process to have people look back at what's developped as a unit of code, and judge it with some distance, possibility from another team to create even more distance from the code.
That's where some teams put the checklists, or force a recheck of the requirements to validate the code actually matches what we want, etc.
So sure, if someone only cares about velocity PRs are just a burden, but that's far from a consensus IMHO (I'll show my hand by confessing I open PRs for myself, on my own private project, just to review the code in another context than my editor...)
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 misunderstood the requirements and implemented something not quite right, or didn't implement your tests properly and thus are not actually testing what you think you are.
CI isn't some magical fairy that ensures program correctness; it's only as good as the tests it runs. And even the best tests do not guarantee correctness.