4 ms·

I've been labelled controversial (amongst other things..) in the past for stating this but hey ho, I'll take another leap: Aside from the "small changes, fast

by Jenk 5y ago

I've been labelled controversial (amongst other things..) in the past for stating this but hey ho, I'll take another leap: Aside from the "small changes, fast fixes" type of mantra, and "pairing is better than reviewing" that I suspect Martin Fowler is leaning on (and that I both support strongly): There is a difference between having reviews, and enforcing reviews. In my multi-decade career I'm yet to have a signle instance of the thought "thank god we prohibit changes that haven't been reviewed by two other people" But I have lost count the number of times I've had the thought "I need to bypass this policy because shit's on fire and I've got a fix" Then comes the question of quality of review when they are mandated. I'll interject with: Code reviews are no inherently bad. Feedback is good. Collaboration is good. There are better methods of providing feedback than code reviews (I strongly object to the post-facto nature of code reviews) Mandated peer reviews less so. Feedback is hurried, if not entirely absent, as it becomes another task that people _have_ to do. The time spent reviewing is rarely accounted for. There's a lot of knowledge transfer required for any non-trivial review to be effective, further increasing the demand on the participants. Code reviews are another tool in the box, but they should not be used for every job.

pierrebai 5y ago
Where I currently work, we have "skip review" and "skip preflight" labels for this. The mergers have the power to merge anything anyway, the labels are only to make it an official request.
- westurner 5y ago
  > Where I currently work, we have "skip review" and "skip preflight" labels for this. The mergers have the power to merge anything anyway, the labels are only to make it an official request. From the OP: > Changes are categorized as either Ship (merge into mainline without review), Show (open a pull request for review, but merge into mainline immediately), or Ask (open a pull request for discussion before merging).
lowercased 5y ago
My code reviews of others is often "please add a test", then nothing happens for 3 days, and I get people saying "when will this be done? it already passed UAT! get it moving". then stuff gets merged without tests, and the cycle repeats. Not ALWAYS, and commit/merge absent tests is sometimes required. The shoe was on the other foot a few years back and I was contracted on part of a large/legacy enterprise app product. I could 'fix' a bug, but couldn't always write a test, as I just didn't understand the system well enough to grok what was there. Policy was "you have to have a test". Fair enough. I remember hitting up 5-6 other devs on the team - some of whom had been there for 10 years - to pair for an hour or so to help me write a test. The same people who were blocking the PR requiring a test would not help me write a test. Some were honest enough to say "I don't know how you'd test that". But... it was frustrating. I had a 20 minute code patch (which demonstrably fixed the bug to the PM's satisfaction) take 8 calendar days to figure out how to test. Then a reviewer didn't like how the test setup was done. When they learned it was done with code from a colleague (and not of my own doing) they suddenly liked it and said it was 'clever'. Happy happy days...
- dopidopHN 5y ago
  I think I left your previous work place a few month ago. It’s just too similar. Each point.
  - lowercased 5y ago
    was the company in the southeast US? :)
- mixmastamyk 5y ago
  Contractors are generally paid by the hour. So while frustrating, there's always the paycheck to look forward to.
- ryan_lane 5y ago
  This is why having tests shouldn't be enforced by review, but if the code coverage drops for the PR, it fails tests.
cratermoon 5y ago
> "I need to bypass this policy because shit's on fire and I've got a fix" Want a list of catastrophes that occurred because someone did this? Edit: Also, Fowler didn't write this article, it's just on his site.
- Jenk 5y ago
  Want a list of absolutes that started as just examples? I could write a long, long list of examples that were, in essence if not literally, typos that were missed by code review but bombed when in prod, too.
  - cratermoon 5y ago
    You mean like Mariner I? "The post-flight review also showed that a missing hyphen (technically a bar in the variable r̄) in coded computer instructions in the data-editing program allowed transmission of incorrect guidance signals." https://www.edn.com/mariner-1-destroyed-due-to-code-error-july-22-1962/ https://www.edn.com/mariner-1-destroyed-due-to-code-error-ju...
    - Jenk 5y ago
      Or Arianne 5[0] Both had code reviews. Both catastrophically failed. No I am not trying to draw causation that code reviewed software ends in tragedy :) [0]: https://www.bugsnag.com/blog/bug-day-ariane-5-disaster https://www.bugsnag.com/blog/bug-day-ariane-5-disaster
      - cratermoon 5y ago
        The Ariane software failure was slightly different because it only failed in the context of the differences in hardware between Ariane 1 and Ariane 5. It was perfectly valid code provided it was used within the constraints for which it was first designed. The Mariner code was simply wrong, and was destined to fail even under the conditions for which it was intended. It's a subtle difference, but I think an important one, especially when considering the complexities for program proofs. I'm sort of (not eagerly) awaiting the day when something written in Rust is implicated in a catastrophic failure. All the platitudes about safety and guarantees will smack hard against a wall like Wile E. Coyote in a Road Runner flick.
mixmastamyk 5y ago
I get the feeling that perhaps you haven't seen code review done well. > lost count the number of times ... "I need to bypass this policy because shit's on fire and I've got a fix" The problem is elsewhere. Asking the "Five Why's" might help locate it.
- nitrogen 5y ago
  I've seen some pretty tortured answers to the five why's to avoid having to ask the really uncomfortable questions about a team/org/etc.
Matthias247 5y ago
When "shit's on fire" usually multiple people are aware about it. In that case some of those should be able to jump in as reviewers and approve the changes without too much drama. In my experience that works fairly well - but it obviously depends on the team. In such code reviews it can also help to point out > this is a hotfix for issue XYZ. A better fix / more tests / etc will follow" to make others aware why a change might not meet all best practices. And obviously if that is the case, the follow-up later on should really happen in order to avoid losing trust from reviewers.

by Jenk 5y ago

pierrebai 5y ago

Where I currently work, we have "skip review" and "skip preflight" labels for this. The mergers have the power to merge anything anyway, the labels are only to make it an official request.

westurner 5y ago
> Where I currently work, we have "skip review" and "skip preflight" labels for this. The mergers have the power to merge anything anyway, the labels are only to make it an official request. From the OP: > Changes are categorized as either Ship (merge into mainline without review), Show (open a pull request for review, but merge into mainline immediately), or Ask (open a pull request for discussion before merging).

lowercased 5y ago

My code reviews of others is often "please add a test", then nothing happens for 3 days, and I get people saying "when will this be done? it already passed UAT! get it moving". then stuff gets merged without tests, and the cycle repeats. Not ALWAYS, and commit/merge absent tests is sometimes required. The shoe was on the other foot a few years back and I was contracted on part of a large/legacy enterprise app product. I could 'fix' a bug, but couldn't always write a test, as I just didn't understand the system well enough to grok what was there. Policy was "you have to have a test". Fair enough. I remember hitting up 5-6 other devs on the team - some of whom had been there for 10 years - to pair for an hour or so to help me write a test. The same people who were blocking the PR requiring a test would not help me write a test. Some were honest enough to say "I don't know how you'd test that". But... it was frustrating. I had a 20 minute code patch (which demonstrably fixed the bug to the PM's satisfaction) take 8 calendar days to figure out how to test. Then a reviewer didn't like how the test setup was done. When they learned it was done with code from a colleague (and not of my own doing) they suddenly liked it and said it was 'clever'. Happy happy days...

dopidopHN 5y ago
I think I left your previous work place a few month ago. It’s just too similar. Each point.
- lowercased 5y ago
  was the company in the southeast US? :)
mixmastamyk 5y ago
Contractors are generally paid by the hour. So while frustrating, there's always the paycheck to look forward to.
ryan_lane 5y ago
This is why having tests shouldn't be enforced by review, but if the code coverage drops for the PR, it fails tests.

cratermoon 5y ago

> "I need to bypass this policy because shit's on fire and I've got a fix" Want a list of catastrophes that occurred because someone did this? Edit: Also, Fowler didn't write this article, it's just on his site.

Jenk 5y ago
Want a list of absolutes that started as just examples? I could write a long, long list of examples that were, in essence if not literally, typos that were missed by code review but bombed when in prod, too.
- cratermoon 5y ago
  You mean like Mariner I? "The post-flight review also showed that a missing hyphen (technically a bar in the variable r̄) in coded computer instructions in the data-editing program allowed transmission of incorrect guidance signals." https://www.edn.com/mariner-1-destroyed-due-to-code-error-july-22-1962/ https://www.edn.com/mariner-1-destroyed-due-to-code-error-ju...
  - Jenk 5y ago
    Or Arianne 5[0] Both had code reviews. Both catastrophically failed. No I am not trying to draw causation that code reviewed software ends in tragedy :) [0]: https://www.bugsnag.com/blog/bug-day-ariane-5-disaster https://www.bugsnag.com/blog/bug-day-ariane-5-disaster
    - cratermoon 5y ago
      The Ariane software failure was slightly different because it only failed in the context of the differences in hardware between Ariane 1 and Ariane 5. It was perfectly valid code provided it was used within the constraints for which it was first designed. The Mariner code was simply wrong, and was destined to fail even under the conditions for which it was intended. It's a subtle difference, but I think an important one, especially when considering the complexities for program proofs. I'm sort of (not eagerly) awaiting the day when something written in Rust is implicated in a catastrophic failure. All the platitudes about safety and guarantees will smack hard against a wall like Wile E. Coyote in a Road Runner flick.

mixmastamyk 5y ago

I get the feeling that perhaps you haven't seen code review done well. > lost count the number of times ... "I need to bypass this policy because shit's on fire and I've got a fix" The problem is elsewhere. Asking the "Five Why's" might help locate it.

nitrogen 5y ago
I've seen some pretty tortured answers to the five why's to avoid having to ask the really uncomfortable questions about a team/org/etc.

Matthias247 5y ago

When "shit's on fire" usually multiple people are aware about it. In that case some of those should be able to jump in as reviewers and approve the changes without too much drama. In my experience that works fairly well - but it obviously depends on the team. In such code reviews it can also help to point out > this is a hotfix for issue XYZ. A better fix / more tests / etc will follow" to make others aware why a change might not meet all best practices. And obviously if that is the case, the follow-up later on should really happen in order to avoid losing trust from reviewers.