3 ms·

so, it's not just a refactoring then; it's also bug fixes + refactoring. In my experience, those are the worst PRs to review. Either just fix the bugs, or just

so, it's not just a refactoring then; it's also bug fixes + refactoring. In my experience, those are the worst PRs to review. Either just fix the bugs, or just refactor it. Don't do both because now I have to spend more time checking the bugs you claim to fix AND your refactoring for new bugs.

rcxdude 2y ago
There are certainly classes of bugs for which refactoring is the path of lowest resistance
- t-writescode 2y ago
  And there are multi-PR processes that can be followed to most successfully convert those changes in a comprehensible way. It'll often include extra scaffolding and / or extra classes and then renaming those classes to match the old classes' name after you're done, to reduce future cognitive load.
  - rcxdude 2y ago
    I'm unconvinced that adding extra code churn in order to split up a refactor that fixes bugs into a bugfix and a refactor is worthwhile
    - withinboredom 2y ago
      One metric I like to give my team is to have any new PR start a review in less than 15 minutes and be completed within 15 minutes. So, the longest you should wait is about 30 minutes for a review. That means teams either go "fuck it" and rubber stamp massive PRs -- which is a whole different issue -- or they take it seriously and keep PRs small to get their PRs reviewed in less than 30 minutes. In most cases where I see responses like this, they're not surprised to wait hours or days for a PR review. In that case, it makes sense to go big, otherwise you'll never get anything done. If you only have to wait half an hour, max, for a PR review; the extra code churn is 1000% worth it.
      - t-writescode 2y ago
        This is where my stance is. As a developer, I want my PRs to actually be reviewed by my coworkers and to have issues caught as a second layer of defense, etc. As a reviewer, I effectively stopped approving things I couldn't give at least a cursory, reasonable glance (and tried to encourage others to follow suit because if we're not reviewing things, why not just push directly to main). As a consequence, I have: * tried to review most things within like half an hour of their announcement in the shared MR channel * requested a pair programming session and offered to do a pair programming session for any large and semi-or-fully automated refactoring session, like running a linter or doing a multi-file variable rename (the pair programmer immediately comments on and approves the MR when it appears) * tried to limit my PRs to approximately 400 lines (not a rigid rule) There were some specific instances of people not liking the "you must pair program if you're going to touch 400 files in one PR" requirement; but otherwise, I would like to think those on my team liked the more regular PRs, more people doing the PRs, etc, that resulted from this and some healthy culture changes. I would also like to feel like the more junior devs were more willing to say anything at all in the PRs because they could follow the change.
        withinboredom 2y ago
        I’ve seen this and variations done by teams to implement the metric. Usually, the “biggest” friction comes from “how do we know a PR needs to be reviewed within the time frame?” To which I always want to answer: “you have a mouth, put noises through it.” Sigh, sometimes I miss the military… anyway, toxic behavior aside, this is usually the biggest thing. I have to remind them that they go get coffee or smoke at least every hour, but rarely at the same time; so maybe then might be a good time to just do a quick check for an open PR. Or turn on notifications. Or if it’s urgent, mention it in the dev team channel. But yeah, it’s hard to get the culture rolling if it isn’t already in place nor has anyone in the company worked with a culture like that.
      - rcxdude 2y ago
        I'm all for low-latency reviews, but this target seems crazy: a perfect recipe for a lot of apparent activity for little actual progress. Maybe it depends on the project, but for a lot of projects 15 minutes of review time means you basically are only going to accept trivial changes.
- edflsafoiewq 2y ago
  The most common IME are bugs that come from some wrong conceptual understanding underpinning the code. Rewriting the code with a correct conceptual understanding automatically fixes the bugs.
  - ludston 2y ago
    The classic example of this is concurrency errors or data corruption related to multiple non-atomic writes.
lazyasciiart 2y ago
No, I very deliberately did not describe any bug fixes.

rcxdude 2y ago

There are certainly classes of bugs for which refactoring is the path of lowest resistance

t-writescode 2y ago
And there are multi-PR processes that can be followed to most successfully convert those changes in a comprehensible way. It'll often include extra scaffolding and / or extra classes and then renaming those classes to match the old classes' name after you're done, to reduce future cognitive load.
- rcxdude 2y ago
  I'm unconvinced that adding extra code churn in order to split up a refactor that fixes bugs into a bugfix and a refactor is worthwhile
  - withinboredom 2y ago
    One metric I like to give my team is to have any new PR start a review in less than 15 minutes and be completed within 15 minutes. So, the longest you should wait is about 30 minutes for a review. That means teams either go "fuck it" and rubber stamp massive PRs -- which is a whole different issue -- or they take it seriously and keep PRs small to get their PRs reviewed in less than 30 minutes. In most cases where I see responses like this, they're not surprised to wait hours or days for a PR review. In that case, it makes sense to go big, otherwise you'll never get anything done. If you only have to wait half an hour, max, for a PR review; the extra code churn is 1000% worth it.
    - t-writescode 2y ago
      This is where my stance is. As a developer, I want my PRs to actually be reviewed by my coworkers and to have issues caught as a second layer of defense, etc. As a reviewer, I effectively stopped approving things I couldn't give at least a cursory, reasonable glance (and tried to encourage others to follow suit because if we're not reviewing things, why not just push directly to main). As a consequence, I have: * tried to review most things within like half an hour of their announcement in the shared MR channel * requested a pair programming session and offered to do a pair programming session for any large and semi-or-fully automated refactoring session, like running a linter or doing a multi-file variable rename (the pair programmer immediately comments on and approves the MR when it appears) * tried to limit my PRs to approximately 400 lines (not a rigid rule) There were some specific instances of people not liking the "you must pair program if you're going to touch 400 files in one PR" requirement; but otherwise, I would like to think those on my team liked the more regular PRs, more people doing the PRs, etc, that resulted from this and some healthy culture changes. I would also like to feel like the more junior devs were more willing to say anything at all in the PRs because they could follow the change.
      - withinboredom 2y ago
        I’ve seen this and variations done by teams to implement the metric. Usually, the “biggest” friction comes from “how do we know a PR needs to be reviewed within the time frame?” To which I always want to answer: “you have a mouth, put noises through it.” Sigh, sometimes I miss the military… anyway, toxic behavior aside, this is usually the biggest thing. I have to remind them that they go get coffee or smoke at least every hour, but rarely at the same time; so maybe then might be a good time to just do a quick check for an open PR. Or turn on notifications. Or if it’s urgent, mention it in the dev team channel. But yeah, it’s hard to get the culture rolling if it isn’t already in place nor has anyone in the company worked with a culture like that.
    - rcxdude 2y ago
      I'm all for low-latency reviews, but this target seems crazy: a perfect recipe for a lot of apparent activity for little actual progress. Maybe it depends on the project, but for a lot of projects 15 minutes of review time means you basically are only going to accept trivial changes.
edflsafoiewq 2y ago
The most common IME are bugs that come from some wrong conceptual understanding underpinning the code. Rewriting the code with a correct conceptual understanding automatically fixes the bugs.
- ludston 2y ago
  The classic example of this is concurrency errors or data corruption related to multiple non-atomic writes.

lazyasciiart 2y ago

No, I very deliberately did not describe any bug fixes.