4 ms·

To me the biggest time sink in PR reviews is getting the reviewer to open the code review. If I ever take over the world, I'd make a new OS that locks all acti

To me the biggest time sink in PR reviews is getting the reviewer to open the code review. If I ever take over the world, I'd make a new OS that locks all activity whenever you get a code review and doesn't let you switch applications until you click "approve" or "request changes". I find it really crazy how long people take to do reviews. I have Github connected to Slack; if someone needs my review I get a message and can have it done within 5 minutes. (I think I write the most comments on code reviews of anyone in my organization, and I also have the lowest review latency. If I can do it, anyone can do it.)

iwontberude 3y ago
If you link peoples ability to use their computer by how quickly they can click "Approve" on a PR, it will definitely lead to a break down in review quality and beat you at your own personal records of latency and MTTR.
- jrockway 3y ago
  I'm sure there would be negative effects, but at least things would move forward quickly. This is the worst system except for all the others.
  - yjftsjthsd-h 3y ago
    Move quickly yes, move forward not necessarily. You've described a system where everybody hits approve or finds something to complain about as fast as they possibly can without worrying about little questions like whether it should have been approved or whether the changes they request are legitimate.
  - johnnyanmac 3y ago
    Sounds like you work best on a small team if that's your focus. Medoum-large ones already have a proven product (or proven experience). You don't focus on blazing gast iterations at that stage unless you are in early R&D or something evergreen. And well, if you're primary motivation is Financial: okay, you're not being paid to move quickly. Or if you hope to move up the ladder quickly you gotta deal with the office politics. Which includes delays in communication. React to those lapses as you feel is appropriate to your goals and personality.
tgsovlerkhgsel 3y ago
Everyone thinks their task is the most important one. If the "lock OS until review done" thing were a thing, there's a good chance that "lock OS until random bullshit someone else wants you to do" would keep you from sending out the code for review in the first place. And even though it's "just a quick thing", if every time you're trying to get something done some "quick thing" gets in the way, it also gets incredibly frustrating. [1][2] Finding a good balance for this is hard. What helps in my experience is building a reputation for small, easy to review changes. I know that I've left reviews from one coworker sitting for a long time because I either already took a look and knew that it was, or hadn't taken a look and dreaded it to be, a huge block of code that would require significant comments. [1] https://www.reddit.com/r/ProgrammerHumor/comments/2rmir6/why_developers_hate_being_interrupted/ https://www.reddit.com/r/ProgrammerHumor/comments/2rmir6/why... [2] https://www.reddit.com/r/ProgrammerHumor/comments/pafo1v/understanding_developer_interruptions/ https://www.reddit.com/r/ProgrammerHumor/comments/pafo1v/und...
yjftsjthsd-h 3y ago
> I have Github connected to Slack; if someone needs my review I get a message and can have it done within 5 minutes. Do you not do anything else? If I'm working on something else, I'm not getting to any new task within 5m (at least, not without a net loss of productivity to reload context)
- withinboredom 3y ago
  If you're writing code and you get a review request and ignore it, you are literally preventing value from being shipped while you work on making some abstract idea a reality. Usually, the code review is the very last step before value becomes available to the business. There is absolutely no reason to delay a code review in this case, none, whatsoever. Even if it isn't the last step, you're delaying the chance for a developer to continue working. More often than not, when a PR is submitted, there is a period where the submitter is twiddling their proverbial thumbs to prevent context switching from a reviewer. So, delaying a review prevents value from being available and reduces overall developer productivity for the entire team. Or, keep working on making an imaginary idea into a real one while someone else's already built idea sits there wasting money.
  - johnnyanmac 3y ago
    >There is absolutely no reason to delay a code review in this case, none, whatsoever. "I'm in a meeting with business owners". There, now either value is delayed or egos from the ones gaining value is bruised. Guess which one usually prevails? We don't have to be this dramatic here.
  - yjftsjthsd-h 3y ago
    > Usually, the code review is the very last step before value becomes available to the business. There is absolutely no reason to delay a code review in this case, none, whatsoever. Even if I was just working on another feature, that's questionable, because it only works if earlier-stage work might not ship at all; otherwise a delay at the beginning is the same as delay at the end, and overhead from context switching means you should prioritize whatever you're already doing. But where it really gets absurd to claim that code review is always the highest possible priority is that I never said what I was working on at the time. Maybe I'm working on incident response - "Oh, sorry $BIGGEST_CUSTOMER, I know your prod instance is down and you're losing thousands of dollars per minute, but I just have to pop off for a bit because one of my peers needs me to code review a way to make forms load 0.2 seconds faster!". Maybe I'm working on a different feature, but it's one that will give the company more value (I have been in conversations to the tune of "we need this feature so we can close with this lucrative customer"). Maybe the other thing I'm doing is someone else's code review! Now to be fair, I think there is real value in lowering the turnaround time on reviews. I could even see an argument for having someone who does code reviews to the exclusion of all else (which kinda sounds like the role you're playing in your company), provided that's known and factored in when handing them work. The only reason I think your position is unreasonable is that you've thrown out any notion of triage or nuance and pinned reviews to the very highest priority above all else.
notnullorvoid 3y ago
PR review is an async process, you can't expect your teammates to drop what they are working on in order to review a PR. This is something you can bring up in your stand-up or message channel if the PR has been sitting without a review then kindly ask "hey X, and Y can you give this a review today". Or if it actually is urgent then message someone directly and say it's urgent, and offer to hop on a call to go through the review together so the feedback can be immediate. Actual priority items are always owned by the whole team, not an individual.

iwontberude 3y ago

If you link peoples ability to use their computer by how quickly they can click "Approve" on a PR, it will definitely lead to a break down in review quality and beat you at your own personal records of latency and MTTR.

jrockway 3y ago
I'm sure there would be negative effects, but at least things would move forward quickly. This is the worst system except for all the others.
- yjftsjthsd-h 3y ago
  Move quickly yes, move forward not necessarily. You've described a system where everybody hits approve or finds something to complain about as fast as they possibly can without worrying about little questions like whether it should have been approved or whether the changes they request are legitimate.
- johnnyanmac 3y ago
  Sounds like you work best on a small team if that's your focus. Medoum-large ones already have a proven product (or proven experience). You don't focus on blazing gast iterations at that stage unless you are in early R&D or something evergreen. And well, if you're primary motivation is Financial: okay, you're not being paid to move quickly. Or if you hope to move up the ladder quickly you gotta deal with the office politics. Which includes delays in communication. React to those lapses as you feel is appropriate to your goals and personality.

tgsovlerkhgsel 3y ago

Everyone thinks their task is the most important one. If the "lock OS until review done" thing were a thing, there's a good chance that "lock OS until random bullshit someone else wants you to do" would keep you from sending out the code for review in the first place. And even though it's "just a quick thing", if every time you're trying to get something done some "quick thing" gets in the way, it also gets incredibly frustrating. [1][2] Finding a good balance for this is hard. What helps in my experience is building a reputation for small, easy to review changes. I know that I've left reviews from one coworker sitting for a long time because I either already took a look and knew that it was, or hadn't taken a look and dreaded it to be, a huge block of code that would require significant comments. [1] https://www.reddit.com/r/ProgrammerHumor/comments/2rmir6/why_developers_hate_being_interrupted/ https://www.reddit.com/r/ProgrammerHumor/comments/2rmir6/why... [2] https://www.reddit.com/r/ProgrammerHumor/comments/pafo1v/understanding_developer_interruptions/ https://www.reddit.com/r/ProgrammerHumor/comments/pafo1v/und...

yjftsjthsd-h 3y ago

> I have Github connected to Slack; if someone needs my review I get a message and can have it done within 5 minutes. Do you not do anything else? If I'm working on something else, I'm not getting to any new task within 5m (at least, not without a net loss of productivity to reload context)

withinboredom 3y ago
If you're writing code and you get a review request and ignore it, you are literally preventing value from being shipped while you work on making some abstract idea a reality. Usually, the code review is the very last step before value becomes available to the business. There is absolutely no reason to delay a code review in this case, none, whatsoever. Even if it isn't the last step, you're delaying the chance for a developer to continue working. More often than not, when a PR is submitted, there is a period where the submitter is twiddling their proverbial thumbs to prevent context switching from a reviewer. So, delaying a review prevents value from being available and reduces overall developer productivity for the entire team. Or, keep working on making an imaginary idea into a real one while someone else's already built idea sits there wasting money.
- johnnyanmac 3y ago
  >There is absolutely no reason to delay a code review in this case, none, whatsoever. "I'm in a meeting with business owners". There, now either value is delayed or egos from the ones gaining value is bruised. Guess which one usually prevails? We don't have to be this dramatic here.
- yjftsjthsd-h 3y ago
  > Usually, the code review is the very last step before value becomes available to the business. There is absolutely no reason to delay a code review in this case, none, whatsoever. Even if I was just working on another feature, that's questionable, because it only works if earlier-stage work might not ship at all; otherwise a delay at the beginning is the same as delay at the end, and overhead from context switching means you should prioritize whatever you're already doing. But where it really gets absurd to claim that code review is always the highest possible priority is that I never said what I was working on at the time. Maybe I'm working on incident response - "Oh, sorry $BIGGEST_CUSTOMER, I know your prod instance is down and you're losing thousands of dollars per minute, but I just have to pop off for a bit because one of my peers needs me to code review a way to make forms load 0.2 seconds faster!". Maybe I'm working on a different feature, but it's one that will give the company more value (I have been in conversations to the tune of "we need this feature so we can close with this lucrative customer"). Maybe the other thing I'm doing is someone else's code review! Now to be fair, I think there is real value in lowering the turnaround time on reviews. I could even see an argument for having someone who does code reviews to the exclusion of all else (which kinda sounds like the role you're playing in your company), provided that's known and factored in when handing them work. The only reason I think your position is unreasonable is that you've thrown out any notion of triage or nuance and pinned reviews to the very highest priority above all else.

notnullorvoid 3y ago

PR review is an async process, you can't expect your teammates to drop what they are working on in order to review a PR. This is something you can bring up in your stand-up or message channel if the PR has been sitting without a review then kindly ask "hey X, and Y can you give this a review today". Or if it actually is urgent then message someone directly and say it's urgent, and offer to hop on a call to go through the review together so the feedback can be immediate. Actual priority items are always owned by the whole team, not an individual.