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.
> 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.