4 ms·

I have too many colleagues who rubber-stamp my pull requests. I'm tempted to include a significant mistake in each one to see who's paying attention. Unfortunat

I have too many colleagues who rubber-stamp my pull requests. I'm tempted to include a significant mistake in each one to see who's paying attention. Unfortunately I have done so too many times by mistake with nobody noticing. Maybe if we gamify it a bit and start keeping score. I don't have authority to make a bonus contingent on the result, but it could be money well spent.

kubb 5y ago
I prefer the people who only comment when they have something useful to say. Either way the responsibility for the correctness is with the code author, not the reviewer. Writing whatever and hoping for your coworkers to correct it is equivalent to asking them to do your job.
- hirundo 5y ago
  Disagree. Finding those mistakes is the whole point. When I miss one in someone else's PR, the resulting problems are my fault.
  - auselen 5y ago
    I can never claim I can find all the bugs, I’m not even that smart. Thus I can comment on style, overall compatibility with existing mechanisms. Ask/remind which tests were run. Trivial stuff is easy to catch, refactoring parts I’m familiar might help with pitfalls known to me. Still I can’t see how one can claim finding all bugs is possible.
  - tremon 5y ago
    They're not your fault, they're at most your responsibility. You should be paid triple what your coworkers earn if you're willing to accept fault for what they produce. Are you perhaps a team manager and insisting that all your team PR's go through you? Might I suggest that instead of signing off on all PR's yourself, setting up a good testing framework may help you sleep better at night?
- TrainedMonkey 5y ago
  In my experience there is a scale of how thorough the review can be. The scale is continuous, but most reviews fall into one of three buckets: informational "hey this is what I am doing", light guidance "hey can you check out broad outlines without getting bogged down in details", and coauthors aka "one person writes the code, but all are responsible for the outcome". Generally the bucket review falls in depends on code criticality, code simplicity, company culture, how much reviewer cares about the code you are changing, how much reviewer cares about you/your changes (mentorship etc.), and how tired / overwhelmed the reviewer is. It follows, that assigning reviewers that care about the code/author and not reviewing code while tired tends to produce higher quality reviews. In the end author, reviewer, and thoroughness of the review is an engineering call. Both author and reviewers should be able to understand where they are on the scale and be able to substantiate why.
belter 5y ago
Always assumed the way the loop and game should be played was: Assuming Developers A,B,C,D of somewhat similar experience and expertise. For one month, randomly assign Developer A,B for example, as committers and C and D as reviewers: 1) Developers A and B create code and submit pull requests. 2) Developers C and D are randomly assigned as reviewers. 3) IF there is a future bug, on code that passed the review, its not up to A or B to fix it. One of the originals reviewers, ( ex C or D) are randomly assign to fix the reviewed code. NOT the original Developer. 4) Developers A and B while as committers, get a value metric on how much code passes reviews and gets committed to production. 5) As no Developer A, B, C,or D ever wants to have to fix the code of somebody else...Reviews are very thorough. 6) Next month... A and B are on the reviewers team and C and D on the committers.

kubb 5y ago

I prefer the people who only comment when they have something useful to say. Either way the responsibility for the correctness is with the code author, not the reviewer. Writing whatever and hoping for your coworkers to correct it is equivalent to asking them to do your job.

hirundo 5y ago
Disagree. Finding those mistakes is the whole point. When I miss one in someone else's PR, the resulting problems are my fault.
- auselen 5y ago
  I can never claim I can find all the bugs, I’m not even that smart. Thus I can comment on style, overall compatibility with existing mechanisms. Ask/remind which tests were run. Trivial stuff is easy to catch, refactoring parts I’m familiar might help with pitfalls known to me. Still I can’t see how one can claim finding all bugs is possible.
- tremon 5y ago
  They're not your fault, they're at most your responsibility. You should be paid triple what your coworkers earn if you're willing to accept fault for what they produce. Are you perhaps a team manager and insisting that all your team PR's go through you? Might I suggest that instead of signing off on all PR's yourself, setting up a good testing framework may help you sleep better at night?
TrainedMonkey 5y ago
In my experience there is a scale of how thorough the review can be. The scale is continuous, but most reviews fall into one of three buckets: informational "hey this is what I am doing", light guidance "hey can you check out broad outlines without getting bogged down in details", and coauthors aka "one person writes the code, but all are responsible for the outcome". Generally the bucket review falls in depends on code criticality, code simplicity, company culture, how much reviewer cares about the code you are changing, how much reviewer cares about you/your changes (mentorship etc.), and how tired / overwhelmed the reviewer is. It follows, that assigning reviewers that care about the code/author and not reviewing code while tired tends to produce higher quality reviews. In the end author, reviewer, and thoroughness of the review is an engineering call. Both author and reviewers should be able to understand where they are on the scale and be able to substantiate why.

belter 5y ago

Always assumed the way the loop and game should be played was: Assuming Developers A,B,C,D of somewhat similar experience and expertise. For one month, randomly assign Developer A,B for example, as committers and C and D as reviewers: 1) Developers A and B create code and submit pull requests. 2) Developers C and D are randomly assigned as reviewers. 3) IF there is a future bug, on code that passed the review, its not up to A or B to fix it. One of the originals reviewers, ( ex C or D) are randomly assign to fix the reviewed code. NOT the original Developer. 4) Developers A and B while as committers, get a value metric on how much code passes reviews and gets committed to production. 5) As no Developer A, B, C,or D ever wants to have to fix the code of somebody else...Reviews are very thorough. 6) Next month... A and B are on the reviewers team and C and D on the committers.