Super common. I’m not sure how that’s any different than teams who create a PR, beg someone to review it, they glance it for 3 sounds and leave a “LGTM!” approval so you can finally merge to main.
Same result, whole lot less performative bullshit.
We use branches and reviews for large changes. Small scope changes go directly to main. If it breaks, we rollback and tell that person to stop breaking things.
That doesn't mean you have to block the merge to main. You can do code reviews on the trunk, you can do it on already merged PRs and you can block on the PR until the code review is done. The latter brings a lot of downsides, which we as developers seem to be taking for granted. It looks like developers can't even imagine not working with Github style pull requests.
It depends on your project, but i prefer having a coherent view on all changes on one branch, instead of 20 branches and one outdated trunk.
`lgtm` is a legit typical outcome. biasing to approving, in many companies & software setups, is a great default. ex: we might `lgtm`, or make comments that are suggestions for follow-on PRs without blocking the current.
However... it's also incredibly hard to self-edit and accurately `lgtm` yourself
4 eye principle brings a lot of redundancy. Even if someone just goes lgtm, it prevents people from pushing a change that no other person has seen which means if the original engineer is on holiday you still have someone who is somewhat familiar with the code.
I've seen this approach on smaller teams working at private companies in less-regulated industries. Think a team of 3-5 devs working at a small company.
At publicly traded companies or in regulated industries, yeah, your compliance will require you to disable merging without at least one approval.
Pretty common if you ask me.
Especially, if you work in companies where pair programming is a thing, you do TDD (or at least write tests), have feature toggles, and a CI/CD pipeline in place.
It works fine for a small team (probably three people or fewer), especially when you're still building the product and don't have it deployed yet. Breaking things isn't a major concern at that point. After people have started using your code, or if you hire a bunch of developers, then you probably want to be more careful.
Actually, it works really fine with 30 developers as well. Even from different companies.
Did that a couple of times and it was always way better then having a lot of long living branches.
I'd say branches don't scale with team size.
In the end it's a matter of your project and trust. If you're doing an in house project it's great.
I find it strange how many people can't even imagine working like that, when it does work so well. It's probably a thing you have to experience to see.
This is good advice to surface the way we work, but I don’t see why you would push to main for Ship rather than raising a PR that you merge immediately (Show), if you’re using PRs anyway.
I would like to see a more pragmatic approach to PRs than the Github esque red barrier of doom you see when you enable branch protection and minimum number of reviewers. I trust my team to judge if their change is trivial enough that it doesn't need a second review. As some others have said, if you're using PRs _sometimes_, the cost of running tests even for trivial changes is minimal, so I'd lean towards show and ask, rather than ship.
That being said, I wasn't massive fan of the comparison to continuous integration. At least, I've never used it in that sense. To me, continue integration is the practice of continuously testing your main branch against your dependencies and always testing changes when they are made. Ideally, dependencies are continuously updated too (at least in the case of apps). This is somewhat orthogonal to the code development and review practices that lead to code being on the main branch in the first place.
This method also seems like it may fall victim to the Dunning-Kruger effect; team members who should be "asking", or at least "showing" their work, but default to "ship it" under the impression they know what they're doing and just want to get it done and move on.
I like the concept in theory, but it relies almost entirely on the personal opinion of the submitter, and I'm not sure the submitter is always the best judge of what level of review their own work requires.
> But the adoption of Pull Requests as the primary way of contributing code has created problems. We’ve lost some of the “Ready to Ship” mentality we had when we did Continuous Integration. Features-in-progress stay out of the way by delaying integration, and so we fall into the pitfalls of low-frequency integration that Continuous Integration sought to address.
Feature branches should be pulling/rebasing back on top of master, so the integrate smoothly into main.
> Sometimes Pull Requests sit around and get stale, or we’re not sure what to work on while we wait for review.
This is a sign to me that there's something going on along the lines of:
- the work isn't actually necessary / no one asked for it
- there are more pressing issues demanding others attention, and it's ok that it's sitting there for a while
- the team doesn't feel confident that their comments will be well received
- the team is dysfunctional and unable to communicate the need for a change to the PR
- the team doesn't understand review is one of their responsibilities
- the team is doing sprints, fills their sprint up with 40 hours/person of development time, then forgets to ponder where time for review will come from.
Fix the team communication problem, or anything else you do is also doomed, you just don't know how yet.
> Sometimes they become bloated as we think “well, I may as well do this while I’m here.”
So it hasn't gotten reviewed, may as well make it harder to review, that'll get it somewhere...
Once it reaches a certain size, you need to take a step back and consider how you can chunk it out to make it reviewable incrementally. If you can't do that, examine how it can be factored differently to allow incremental review.
> First – a trick to help you get the best of both worlds – merge your own pull request without waiting for feedback, then pay attention to the feedback when it comes.
Why are people in such a hurry to get it merged? If it's so trivial that it doesn't require a review, it's likely not so impactful that good engineering practice should be ignored.
Without review, I promise that the quality / prevalence of tests will decrease over time, and eventually you wont be able to trust that the one line change that you're making wont break something in production.
Patience and rigor help throughput. This article seems like it's advocating skipping around those because they require communication and get in the way of "getting things done" a. la. cowboy.
The #1 benefit of code review is to ensure that the code being written is understandable to at least one other person. If at least one other person can understand it, it's much more likely several other people will be able to.
In my experience, this is something that many reviewers, including senior engineers, do not understand. If you haven't worked in enough different styles of codebases, maybe you don't even think this is possible or desirable, in the same way that many people didn't think having a uniform formatting style in a codebase was possible or desirable.
When people don't think that everyone has the right to understand what's going on in code just by looking at it, we end up with parts of the codebase that are idiosyncratically written, structured in confusing ways, and very difficult for anyone but 1-2 people who wrote them to maintain. And then when folks review changes by those 1-2 people, they lack the domain knowledge to do a thorough review and tend to just look for surface level issues and go "I don't really have context on this but I assume X knows what they're doing." This should be a red flag for you if you find yourself doing it on a regular basis.
IMO, if your team is operating this way, maybe you should just be writing better tests and merging to master. But consider how your code is evolving. Are there key areas that are only understood by one person on your team? Are there services that you'd rather completely rewrite than take the time to understand? If so, that represents a huge waste of effort. What would prevent that from happening again?
32 comments
[ 2.9 ms ] story [ 87.6 ms ] threadIs that a normal/common practice for anyone here?
TBD works on main, or “trunk” as described in the spec
See https://trunkbaseddevelopment.com/ to know more about it
Same result, whole lot less performative bullshit.
We use branches and reviews for large changes. Small scope changes go directly to main. If it breaks, we rollback and tell that person to stop breaking things.
It depends on your project, but i prefer having a coherent view on all changes on one branch, instead of 20 branches and one outdated trunk.
However... it's also incredibly hard to self-edit and accurately `lgtm` yourself
At publicly traded companies or in regulated industries, yeah, your compliance will require you to disable merging without at least one approval.
Btw, if you want to know more about trunk-based development: https://trunkbaseddevelopment.com/
In the end it's a matter of your project and trust. If you're doing an in house project it's great.
I find it strange how many people can't even imagine working like that, when it does work so well. It's probably a thing you have to experience to see.
That being said, I wasn't massive fan of the comparison to continuous integration. At least, I've never used it in that sense. To me, continue integration is the practice of continuously testing your main branch against your dependencies and always testing changes when they are made. Ideally, dependencies are continuously updated too (at least in the case of apps). This is somewhat orthogonal to the code development and review practices that lead to code being on the main branch in the first place.
Ship / Show / Ask: A modern branching strategy - https://news.ycombinator.com/item?id=28510212 - Sept 2021 (149 comments)
Ship / Show / Ask – A modern branching strategy - https://news.ycombinator.com/item?id=28457071 - Sept 2021 (3 comments)
When you say "changes so big" you think your solution is better.
I am working in an organisation with so called "seniors" and their PRs are approved because other seniors made them friends.
I like the concept in theory, but it relies almost entirely on the personal opinion of the submitter, and I'm not sure the submitter is always the best judge of what level of review their own work requires.
Feature branches should be pulling/rebasing back on top of master, so the integrate smoothly into main.
> Sometimes Pull Requests sit around and get stale, or we’re not sure what to work on while we wait for review.
This is a sign to me that there's something going on along the lines of:
- the work isn't actually necessary / no one asked for it
- there are more pressing issues demanding others attention, and it's ok that it's sitting there for a while
- the team doesn't feel confident that their comments will be well received
- the team is dysfunctional and unable to communicate the need for a change to the PR
- the team doesn't understand review is one of their responsibilities
- the team is doing sprints, fills their sprint up with 40 hours/person of development time, then forgets to ponder where time for review will come from.
Fix the team communication problem, or anything else you do is also doomed, you just don't know how yet.
> Sometimes they become bloated as we think “well, I may as well do this while I’m here.”
So it hasn't gotten reviewed, may as well make it harder to review, that'll get it somewhere...
Once it reaches a certain size, you need to take a step back and consider how you can chunk it out to make it reviewable incrementally. If you can't do that, examine how it can be factored differently to allow incremental review.
> First – a trick to help you get the best of both worlds – merge your own pull request without waiting for feedback, then pay attention to the feedback when it comes.
Why are people in such a hurry to get it merged? If it's so trivial that it doesn't require a review, it's likely not so impactful that good engineering practice should be ignored.
Without review, I promise that the quality / prevalence of tests will decrease over time, and eventually you wont be able to trust that the one line change that you're making wont break something in production.
Patience and rigor help throughput. This article seems like it's advocating skipping around those because they require communication and get in the way of "getting things done" a. la. cowboy.
Most of the articles about working around code reviews give the same vibes as naive crypto enthusiasts towards financial regulation.
I might be he tries so hard to make things his own. Is there anything he does not have an opinion about ?
Fowler isn't the author of this content, Rouan Wilsenach is.
In my experience, this is something that many reviewers, including senior engineers, do not understand. If you haven't worked in enough different styles of codebases, maybe you don't even think this is possible or desirable, in the same way that many people didn't think having a uniform formatting style in a codebase was possible or desirable.
When people don't think that everyone has the right to understand what's going on in code just by looking at it, we end up with parts of the codebase that are idiosyncratically written, structured in confusing ways, and very difficult for anyone but 1-2 people who wrote them to maintain. And then when folks review changes by those 1-2 people, they lack the domain knowledge to do a thorough review and tend to just look for surface level issues and go "I don't really have context on this but I assume X knows what they're doing." This should be a red flag for you if you find yourself doing it on a regular basis.
IMO, if your team is operating this way, maybe you should just be writing better tests and merging to master. But consider how your code is evolving. Are there key areas that are only understood by one person on your team? Are there services that you'd rather completely rewrite than take the time to understand? If so, that represents a huge waste of effort. What would prevent that from happening again?