> 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...)
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 calculations, pull requests are essential.
But is it the most efficient way to ensure that level of quality?
PR-s influence the same code, so it is not enough to review PR-s individually anyway.
Maybe a weekly or monthly release would be a better unit for evaluation.
We have all kind of ways of producing diffs between branches, releases, tags, timestamps, etc?
Why would PR diffs be the most efficient unit to review?
Often PR-s are just a silly way to communicate between team members instead of just talking to each other. I.e., you sit in the same office and create and reject PR-s, when you could just say to you colleague: "if you do not like the name of that variable, feel free to change the name".
The main problem with the PR system is that changes to the code is not immediately committed to the branches that developers work on, so it limits cooperation between developers. You can still review commits to dev branches and if problems are found, you can fix them or in most cases undo them.
I don't disagree with everything in there, The trouble with pull requests for instance is very real, but yeah, without mentioning the trouble with continuous integration nor the benefits of pull requests, it is just a pretty weak and one-sided article. Especially developers should now better than state 'B is bad because xxx and A is good because yyy so pick yyy'.
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.
You could make the same argument about automation not manual process.
e.g. "A bug in production can literally cost us billions (apart from potential reputation damage), the system is highly complex, so having robust test suites, automated monitoring and rollback isn't a luxury for us but a necessity."
IMHO, automation wins every time. That is, while there is value in "stop the pipeline versions of code review", we value the automation more.
That is a perfectly acceptable alternative, but to me it feels it would put a lot of trust in automation and lower affinity with the code as it's not continuously inspected. It depends on the size and duration of the project I guess.
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.
> An outsider may not understand the vision and direction of your project. They may not have the same habits and norms for testing, code quality, and style. However, your own team members should share these norms.
Yeah, no. The challenge of running a company is that even within your company, everyone becomes misaligned the moment you’ve aligned. Have you hired anyone new? They don’t understand the company’s vision, nor have internalized your engineering standards. And even once they’ve worked with you for years, they still won’t have aligned on the things that they never ran into, AND they’ll misalign on all the things they've developed opinions on.
> Doing this means that a human only spends time reviewing code that has already been proven technically correct.
Yes, CI is useful for ensuring technical correctness. But technical correctness is one very small aspect of reviewing, and usually not where the big issues come in day to day.
Getting eyes on code is not about preempting things that would fail CI anyway. You probably should be running your full CI suite on PRs, too. To me it’s about catching misalignments on what the product should do, broader misalignments on how it should be engineered, catching misalignments that someone who’s too deep in the weeds would miss, talking through issues realized after the requirements had already been set and discussed, and more. A CI pipeline can’t fix that, pair programming will only address the things that an engineering peer who is as equally in the weeds as the contributor is, and integrating the newest things to production without human review is nice if your product is sufficiently low stakes that subjecting users to the effects of these people problems is okay; but that’s not for me and my team.
Pushing faster is not a pure good without trade offs.
I respectfully disagree. I worked with "trunk"s before, won't do that again, sorry.
I would argue that Pull Requests (or Merge Requests) are the single most efficient practice to ensure the quality of the code in a project with many people, right after the CI with linters, formatters, tests, etc. which you should set up even if you work alone (at least running them locally). Not to mention the knowledge sharing aspect of PR/MR reviews. It is a very versatile practice where a reviewer (or several reviewers!) can review the code itself from a different perspective and in a different setting, review the design (or "architecture", if you want), learn something new themselves, and keep up to date with the changes in the source code, but also in a product in general. Even in my solo projects I always take five before committing the code, in order to review it first with a clear head.
Over the years I've experimented with getting rid of pull requests quite a few times. As a whole and solely as an abstract idea I wholeheartedly agree with the article. Code Review is not the same as a pull request.
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.
I think it's good to challenge received best practices like this - even if in this case the alternative seems to hinge on something that most teams struggle with even more -- writing sufficient good tests.
I favour a less automated solution: code previews.
The biggest reason I've seen for code reviews taking a long time, is that they're surprising. If an engineer has implemented a full solution without communicating their intention ahead of time, reviewing it is really hard because the reviewer is often starting from zero.
1. Communicate your plan ahead of time
2. Check in with the team whenever meaningful progress has been made, and during/after key decision points (you can use draft PRs for this).
3. When it's ready to land, hopefully enough incremental reviews have been done that the final review is simple box ticking (and yes, perhaps it can be fully automated with CI).
Essentially, I think getting humans to look at your code is important, but it doesn't have to be done in a way that feels like a roadblock.
A pull-request based approach is more sane than changing CI/CD to CI/C-MergeConflicts
"pull requests sacrifice performance, including both delivery time and quality"
Delivery time does not matter if your feature is not fully implemented; "Your team members should share your norms for quality" (paraphrased)
The article mentions having "regular, scheduled reviews" which sound like a chore and break flow more than having to review a PR every now and then. Having these on a weekly basis (as the article suggests) while having everyone push daily means you have 5 days of incomplete, unchecked code in your codebase, per engineer, every week.
CI and PRs are not mutually exclusive, you can have your github PR in "draft-mode" and it should still run the same workflows. Have your tests be automated and when the feature is done and passing; you request a review.
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.
I've used quite extensively Pair Programming, and at its best is better when both people tend to be senior, as you can bounce design ideas out of each other much easier, and is more likely that a bad decision (or test, or ...) gets caught earlier.
With junior devs is too easy to become something in which the senior dominates the conversation.
> 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.
It is very tiring. When i started doing it, i was amazed how tired i felt at the end of a 9 to 5 workday. But i got used to it, and it wasn't long before it was perfectly comfortable. I think there's some sort of mental muscle you exercise by pairing, and it takes a while to develop the endurance it needs.
I do wish pairing advocates talked about this. It feels mildly fraudulent not to.
I'm not sure introversion vs extraversion is relevant. Firstly, i'm not sure they're even real things; they show up in a lot of snake oil psychology (Jung, Myers-Briggs), but i don't know to what extent they label a real axis of variation. But if extraversion means "the state of primarily obtaining gratification from outside oneself", then an extravert ought to hate pairing, because it's the same as programming solo, but with someone constantly pointing out what you've done wrong. The conversation you have during pairing is very, very different to a normal social conversation, because it's so focused on the work in hand.
> I'm not sure introversion vs extraversion is relevant. Firstly, i'm not sure they're even real things; they show up in a lot of snake oil psychology (Jung, Myers-Briggs)
Calling Jung snake-oil psych is a bit of a hot take!
Outside of that, Introversion/Extroversion is part of the Big 5 personality traits which I understand to be widely accepted and (unlike a lot of psych) replicated multiple times.
In any case, at the end of a day of pairing I find myself exhausted and have basically no desire to communicate with anyone for the rest of the day. Maybe that's something I'd get used to eventually, though I'm doubtful.
Even a couple of hours of pairing per day is a huge team productivity boost though. Don't do it "all day every day".
It's OK to say "I'll go away for the afternoon to flesh out these tests as we have discussed".
I'll say that it's less about if the PR exists or not, but by the time it's raised it should be fine to merge it pretty much as soon as it's green, because people are on the same page before the code was even written.
> “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.
In our setup, the committer can merge their pull request after CI passed and at least one review has approved the code. Seems like a better compromise than relying solely on CI. And pairing is nice, but there's not always someone available to pair with. Heck, I always review my own pull requests completely before requesting a review, because I often miss something. And the easiest way to do this, that I know of, is with pull requests.
I get what is being said. There can be a delay in waiting for a review, but in my experience that's more an issue of bad discipline. I do get the point about pair programming making reviews near-redundant, though I'd be inclined to still get review from a third party.
I also think pull requests in github, as an actual place you can go and comment on and discuss code, is such a useful tool for doing reviews in a public environment where everyone on a team can go back and refer to the discussions that I'd be inclined to accept some amount of delay as an acceptable cost.
Agreed. It's part of my job to review my colleagues code, and I expect reviews from them ideally within a couple hours, maximum 24 hours. I find this practice also helps with knowledge sharing and breaking silos. And while I wait for a review, I can start another task, do some manual QA, grab a coffee, write some docs for this feature, etc. I rarely find myself stuck waiting for reviews.
My biggest issue is really that github PRs are shit, and especially for large PRs they scale very badly (though that’s also an issue of having large PRs):
- it’s extremely hard to correlate changes to comments, especially when rebase / force push are involved
- long discussions are extremely hard to follow as they’re not threaded, especially as “technical” events get mixed with people talking, you can easily have more noise from technical changes (tags, pushes, issues / PRs linking to the PR, review requests) than you have human comments, and GitHub’s forceful folding of the middle of the discussion makes things worse.
- inline comments quickly make diff views unreadable
- there’s no good / built-in handling of individual concerns to resolve, you can “resolve” inline annotations but you have to visit individual resolved threads and pray the author left some indication as to how they resolved the thing otherwise you’ve got no idea
I 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.
Can 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.
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 after.
We work in a regulated industry and having a 2nd party who is required to review code prior to merging is essentially mandatory. We can produce high-quality logs and provide them for inspection. Every single line of code in our codebase has had at least 2 humans review it.
Pull requests are really awesome. Slowing certain things down on purpose can help you go a lot faster.
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?
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.
I joined a company last year that has a handful of legacy products, and we successfully moved one of them to a CD model a few months ago.
It's hilarious to sit back and watch every other team have the same monthly loop of big change -> lengthy test cycle -> "go-live" meeting -> fixes/big change -> more testing... etc. Loads of stress and finger-pointing.
Hardly ever hear a peep from the CD team. Sure, it's not all rainbows and lollipops. They do have urgent production issues time-to-time, but most are resolved by a rollback (or feature toggle flipped).
Congratulations. But it might be time to "make a peep" internally to other teams about "how we successfully moved to a CD model and what it gave us, and how you can do it too".
Mainly for your colleague's stress levels, and secondarily for the business's success.
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.
The term "continuous deployment" was absolutely coined to describe automatically deploying the trunk into production after it passes some set of tests. That's a sufficiently scary idea that that for many people the meaning has become watered down over time.
Note that i'm not endorsing the practice. I find it scary too.
This is called "continuous deployment", and it was all the rage ten years ago. I'm surprised the idea seems unfamiliar now, but that's how this industry works, i suppose!
A first requirement for direct-to-main development is that you can be reasonably sure your code will not break the CI build otherwise you'll ensure perpetually broken builds and a process many times slower than PR:s.
Because that’s what PRs are mostly: validation that they won’t break the build on main. Reviews aren’t strictly necessary but compiling is.
In some small project you might be able to run compile/test as part of a commit hook but for a situation where builds might take several hours or the developer can’t test all platforms that CI will, then it’s only possible to keep a green main by validation. In that scenario reviews wouldn’t be the limiting factor either.
Hm, the section on using CI rather than pull requests seems odd. PRs can be part of CI. You can set up your pipeline to build and test the result of a PR; that is, you are testing each change as if you had pushed it to the project's mainline, but there's no potential for you to break the mainline branch in a way that hinders your team.
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.
> Hm, the section on using CI rather than pull requests seems odd. PRs can be part of CI. You can set up your pipeline to build and test the result of a PR; that is, you are testing each change as if you had pushed it to the project's mainline, but there's no potential for you to break the mainline branch in a way that hinders your team.
That doesn't actually stop you from breaking mainline though, just make it significantly less likely.
Say you are developing a class Foo, and I'm adding new code that uses Foo. You decide to refactor Foo to Bar. We won't get a conflict because I'm adding new code and not touching the Foo class.
In that case we both have pull requests that build fine but that once merged, won't compile. Running pipelines on each PR reduces the breakage but it can't eliminate it, unless you only allow one PR to build then merge at a time.
I usually have two jobs on every PR: one that builds "as presented", and one that builds after an automatic integration with the parent branch. If the merge fails, you still have test feedback from the "as presented" build.
Depending on how long your builds are it doesn't even really take any extra time, if they're done in parallel
Git was designed around the idea of a clean history. Now, one might claim that "designed around the idea" does not necessarily mean that it doesn't work if you don't keep a clean history. However, there's a large amount of tooling within git which outright breaks if you don't stick to a clean history. As such, I am just going to go out and state that if you don't want to keep a clean history, you're at best not getting as much out of git as you reasonably could, and at worse, getting less out of git than if you were to use an alternative which was designed around the idea of not keeping a clean history (e.g. fossil).
In summary, even though I often see software projects already doing a bad job of maintaining a clean history already (which is partly because of pull requests being the wrong tool for small change sets (see kernel development for situations where pull requests make sense)), this approach is even worse.
Want continuous integration to work best? Split up the work into smaller parts which can be contributed as smaller individual units (whether you use PRs or some alternative system). Continuously pushing incomplete changes (even if you round them off so tests pass and nothing immediately breaks) is just not compatible with git.
This blog post reads like it's written by someone using Subversion back in 2015. Most of the justification for committing on mainline can be addressed by following other best practices:
1. Run the same suite tests additional quality checks (linters, etc) on PR branches that you run on mainline
2. Run branch CI against the merge commit for your PR branch (i.e. include your changes as well as latest mainline)
3. Avoid long-lived branches.
If you run into frequent merge conflicts and incompatible changes in an area of your codebase such that your team needs to integrate individual commits every half-hour instead of keeping 1-4 day feature branches around... That sounds like a code and/or organizational smell.
However!
> 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.
I actually do like this analogy -- but it's not PRs that are the problem, it's mandatory reviews. And the cost/benefit of mandatory reviews varies widely depending on how senior the engineers on the team are, how much their work overlaps, and what their average tenure on the team is.
Where I work we have two code bases. The older of the two in TFS and the newer in git. In TFS we use trunk-based development and in git we do pull requests.
I must say that I very much prefer having a formal, tool-protected, review process, doing integrations and testing on a separate branch from the main branch to allowing developers like we do in git to the added ease of being available to commit directly to trunk.
Even with CI to run tests upon committed code you never want to leave your mainline branch in a broken state. No matter how fast the feedback is, because you don't really know how fast your developers will be to act on that feedback!
Some good points! There are, however, also changes that you do not want to propagate to your teammates or production unless you are 100% sure that they are the final solution to the problem you are trying to solve. Sometimes, the complexity of a problem is so high that you have to experiment with the code.
When those changes include non-reversible database migrations, for example, it is better not to integrate that code into "mainline" before you've settled on a specific implementation.
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.