Ship / Show / Ask (2021)

I'm confused by this. I've never worked in an organization where "commit directly to main" would get you anything but in a ton of trouble.

Is that a normal/common practice for anyone here?

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.

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?

Related:

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)

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 article feels a lot patronizing than anything useful.

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.

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

I can't tell you what it is but I have such a hard time reading content on Martin Fowlers site.

I might be he tries so hard to make things his own. Is there anything he does not have an opinion about ?