6 ms·

This is how I see the pull request example with credit cards and traveling. You are on your way to a conference. You are at the airport and want to buy a coffe

This is how I see the pull request example with credit cards and traveling. You are on your way to a conference. You are at the airport and want to buy a coffee and a donut. You make a request... You've been waiting for almost an hour now. Nobody takes a look at and you still need multiple approvals, so you ping Barbara in charge of approvals to get the ball rolling, she says she will take a look after a meeting she is currently attending because she wants to be careful with what she approves. Oh, Tom, her colleague asks you why don't you eat salad and orange juice, it's cheaper and faster. You have a quick call with Tom to explain why you want a coffee and donut. Awesome, you convinced Tom to approve in just under 20 minutes. Barbara is back, but she doesn't respond... She is MIA, she went for breakfast... Your plane is leaving, so you skip breakfast. You arrive, by the time you land Barbara approved your breakfast, but now you are in a new country and circumstances changed, so you update your request. The earlier approvals are now invalid. You go through the whole charade again... Pull requests assume you cannot trust your colleagues to make reasonable choices and you also don't trust your automated tests to catch anything insane (buying a 100k car for breakfast in the above example), so you have to gatekeep and slow down your team to make sure (?) that the code they commit is good.

wizofaus 3y ago
I don't assume my colleagues will always make "reasonable" choices and furthermore correctly express those choices into code any more than I will myself! In fact perhaps most of the issues I find in PRs are after reviewing my own ones (often it's silly things like accidentally including files/commits I shouldn't've have). Plus knowing someone else must've approved my PR stops me feeling quite so silly if it does end up breaking something :) The breakfast order analogy to me falls down because not being able to merge a PR into the mainline branch immediately is exceedingly rarely something that holds me up - there's always plenty of other stuff to do. And the time I spend reviewing PRs is valuable in its own right, even if I find nothing wrong (understanding the codebase better etc.). Sure the odd PR or two for a truly trivial change slows up the works a little but they're pretty rare and a it's a small price to pay. Having said all that, I'm genuinely interested in what studies might have been done to genuinely guage the effectiveness of the PR/review process. I've certainly worked on some (open source) projects where it clearly was a major drag on productivity, as very few people had approval rights (wherever I've worked professionally all devs have the same rights - single approval from any other dev is always sufficient).
kerkeslager 3y ago
If your entire process is broken and toxic, pull requests aren't going to work for you, but that's not a problem with pull requests, that's a problem with your entire process being toxic and broken. > You make a request... You've been waiting for almost an hour now. 1. Don't design your process so that waiting an hour to merge a pull request is going to be a problem as bad as waiting an hour to eat. Why are you waiting? Move on to the next thing. If it depends on the pending pull request, branch the pending pull request, and work on the new branch--if you have to integrate feedback you can rebase it into the new branch. Occasionally this will cause problems, but they're usually rare. > Nobody takes a look at and you still need multiple approvals, 2. Choose a number of approvals that is appropriate for your team. If you're writing software where the consequences of a mistake are very high that might be a high number, but in most cases in my career, 1 has been a perfectly reasonable number of approvals. In some cases, 0 is the appropriate number of approvals. > so you ping Barbara in charge of approvals to get the ball rolling, she says she will take a look after a meeting she is currently attending because she wants to be careful with what she approves. 3. Don't make one team member in charge of approvals--that's almost guaranteed to make that person a bottleneck. Anyone on the team with the knowledge to evaluate the code effectively should be able to approve it. I've been on one team where one guy was in charge of approvals and wasn't a bottleneck, which was because he did nothing but approvals. That worked great for everyone except him--his life was miserable until we scrapped that idea. > Oh, Tom, her colleague asks you why don't you eat salad and orange juice, it's cheaper and faster. You have a quick call with Tom to explain why you want a coffee and donut. 4. If this sort of exchange is about personal preferences like what you eat or some stylistic nitpicking, then you have to have a team discussion about making sure that we prioritize code review feedback that's actually important and not bikeshedding. But in a lot of cases, Tom is asking because he legitimately doesn't know, and explaining your reasoning to Tom is part of your responsibility to help your teammate keep their understanding of the codebase up-to-date. This isn't wasted time, it's one of the benefits of code review. > You arrive, by the time you land Barbara approved your breakfast, but now you are in a new country and circumstances changed, so you update your request. The earlier approvals are now invalid. 5. If different pull requests depend on each other or are touching the same areas of the code, ideally do them serially rather than working on them at the same time. If they really need to be worked on at the same time, then you need to communicate earlier in the process to be sure you aren't stepping on each other's toes, and it would make sense for the two people working on the same areas of code to review each other's pull requests. Not doing pull requests wouldn't fix this--this is just a challenge of multiple people changing the same codebase. > Pull requests assume you cannot trust your colleagues to make reasonable choices Sometimes reasonable choices are costly mistakes. Even the best coders make mistakes, and unlike a mistake on what food to eat for breakfast, code mistakes can be very costly. > and you also don't trust your automated tests to catch anything insane Automated tests don't catch when your architecture is bad, or when there was a mistake in the specification, or when you've introduced a security vulnerability. Sometimes the mistake caught in code review is in the automated test. > you have to gatekeep and slow down your team to make sure (?) that the code they commit is good. Good code can always be better. I, for one, want people looking at my code to make it better. Obviously there are some tradeoffs here, and code review shouldn't be a massive bottleneck in your process. Done well, code reviews speed up your team: the earlier you catch mistakes the quicker they are to fix.
bluefirebrand 3y ago
> Pull requests assume you cannot trust your colleagues to make reasonable choices You cannot trust any human to make reasonable choices 100% of the time. If people are making good choices and writing good code then the PR process should be super smooth and seamless, right?
kodah 3y ago
There are few reasonable choices in programming. There are a lot of choices with tradeoffs, programmers certainly aren't omnipotent of tradeoffs. Peer review also isn't just about you and your code. A fair amount of the time it's a tool for democratizing knowledge or teaching in the same way that reading incidents occasionally is.

wizofaus 3y ago

I don't assume my colleagues will always make "reasonable" choices and furthermore correctly express those choices into code any more than I will myself! In fact perhaps most of the issues I find in PRs are after reviewing my own ones (often it's silly things like accidentally including files/commits I shouldn't've have). Plus knowing someone else must've approved my PR stops me feeling quite so silly if it does end up breaking something :) The breakfast order analogy to me falls down because not being able to merge a PR into the mainline branch immediately is exceedingly rarely something that holds me up - there's always plenty of other stuff to do. And the time I spend reviewing PRs is valuable in its own right, even if I find nothing wrong (understanding the codebase better etc.). Sure the odd PR or two for a truly trivial change slows up the works a little but they're pretty rare and a it's a small price to pay. Having said all that, I'm genuinely interested in what studies might have been done to genuinely guage the effectiveness of the PR/review process. I've certainly worked on some (open source) projects where it clearly was a major drag on productivity, as very few people had approval rights (wherever I've worked professionally all devs have the same rights - single approval from any other dev is always sufficient).

kerkeslager 3y ago

If your entire process is broken and toxic, pull requests aren't going to work for you, but that's not a problem with pull requests, that's a problem with your entire process being toxic and broken. > You make a request... You've been waiting for almost an hour now. 1. Don't design your process so that waiting an hour to merge a pull request is going to be a problem as bad as waiting an hour to eat. Why are you waiting? Move on to the next thing. If it depends on the pending pull request, branch the pending pull request, and work on the new branch--if you have to integrate feedback you can rebase it into the new branch. Occasionally this will cause problems, but they're usually rare. > Nobody takes a look at and you still need multiple approvals, 2. Choose a number of approvals that is appropriate for your team. If you're writing software where the consequences of a mistake are very high that might be a high number, but in most cases in my career, 1 has been a perfectly reasonable number of approvals. In some cases, 0 is the appropriate number of approvals. > so you ping Barbara in charge of approvals to get the ball rolling, she says she will take a look after a meeting she is currently attending because she wants to be careful with what she approves. 3. Don't make one team member in charge of approvals--that's almost guaranteed to make that person a bottleneck. Anyone on the team with the knowledge to evaluate the code effectively should be able to approve it. I've been on one team where one guy was in charge of approvals and wasn't a bottleneck, which was because he did nothing but approvals. That worked great for everyone except him--his life was miserable until we scrapped that idea. > Oh, Tom, her colleague asks you why don't you eat salad and orange juice, it's cheaper and faster. You have a quick call with Tom to explain why you want a coffee and donut. 4. If this sort of exchange is about personal preferences like what you eat or some stylistic nitpicking, then you have to have a team discussion about making sure that we prioritize code review feedback that's actually important and not bikeshedding. But in a lot of cases, Tom is asking because he legitimately doesn't know, and explaining your reasoning to Tom is part of your responsibility to help your teammate keep their understanding of the codebase up-to-date. This isn't wasted time, it's one of the benefits of code review. > You arrive, by the time you land Barbara approved your breakfast, but now you are in a new country and circumstances changed, so you update your request. The earlier approvals are now invalid. 5. If different pull requests depend on each other or are touching the same areas of the code, ideally do them serially rather than working on them at the same time. If they really need to be worked on at the same time, then you need to communicate earlier in the process to be sure you aren't stepping on each other's toes, and it would make sense for the two people working on the same areas of code to review each other's pull requests. Not doing pull requests wouldn't fix this--this is just a challenge of multiple people changing the same codebase. > Pull requests assume you cannot trust your colleagues to make reasonable choices Sometimes reasonable choices are costly mistakes. Even the best coders make mistakes, and unlike a mistake on what food to eat for breakfast, code mistakes can be very costly. > and you also don't trust your automated tests to catch anything insane Automated tests don't catch when your architecture is bad, or when there was a mistake in the specification, or when you've introduced a security vulnerability. Sometimes the mistake caught in code review is in the automated test. > you have to gatekeep and slow down your team to make sure (?) that the code they commit is good. Good code can always be better. I, for one, want people looking at my code to make it better. Obviously there are some tradeoffs here, and code review shouldn't be a massive bottleneck in your process. Done well, code reviews speed up your team: the earlier you catch mistakes the quicker they are to fix.

bluefirebrand 3y ago

> Pull requests assume you cannot trust your colleagues to make reasonable choices You cannot trust any human to make reasonable choices 100% of the time. If people are making good choices and writing good code then the PR process should be super smooth and seamless, right?

kodah 3y ago

There are few reasonable choices in programming. There are a lot of choices with tradeoffs, programmers certainly aren't omnipotent of tradeoffs. Peer review also isn't just about you and your code. A fair amount of the time it's a tool for democratizing knowledge or teaching in the same way that reading incidents occasionally is.