3 ms·

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

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

emptysongglass 3y ago
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.
- makeitdouble 3y ago
  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...)
masklinn 3y ago
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, …
- xmcqdpt2 3y ago
  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.)
- JenrHywy 3y ago
  100% 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.
  - clawlor 3y ago
    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. https://docs.travis-ci.com/user/pull-requests/#how-pull-requests-are-built https://docs.travis-ci.com/user/pull-requests/#how-pull-requ...
- twic 3y ago
devnullbrain 3y ago
Yes, (competent) code review is constructive, not gladiatorial.
samwillis 3y ago
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.

emptysongglass 3y ago

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.

makeitdouble 3y ago
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...)

masklinn 3y ago

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, …

xmcqdpt2 3y ago
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.)
JenrHywy 3y ago
100% 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.
- clawlor 3y ago
  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. https://docs.travis-ci.com/user/pull-requests/#how-pull-requests-are-built https://docs.travis-ci.com/user/pull-requests/#how-pull-requ...
twic 3y ago

devnullbrain 3y ago

Yes, (competent) code review is constructive, not gladiatorial.

samwillis 3y ago

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.