Refactoring in PRs just makes it more difficult to review. “Do these lines belong to the goal nor not?”. Also, we’re human and miss things. Adding more text to review means the chance of missing something increases.
Especially if the refactored code isn’t just refactored but modified, things are very easy to miss. Move an entire block of code from one file to another and make changes within = asking for trouble or a “LGTM” without any actual consideration. It makes code reviews more difficult, error-prone, and annoying.
Code reviews aren’t there to just tick off a box. They are there to ensure what’s on the tin is actually in it and whether it was done well.
In my experience I haven’t had an issue because usually the refactorings are small. If they’re not I just hop on a call with the person who wrote the MR and ask them to walk me through it.
In theory I’d like to have time to dedicate solely to code health, but that’s not quite the situation in basically any team I’ve been in.
I haven’t had any trouble separating refactors PRs from ticket PRs. Make the ticket PR, make a refactor PR on that ticket PR, merge the ticket PR, rebase refactor PR on master, open ticket PR for review, done 🤷
Had a coworker who was a bit like this. They were tasked to do one simple thing. Required a few lines of code change at most.
They end up refactoring the entire damn thing and introduced new bugs in the process.
Was there much value in the refactoring, like tech debt addressed?
Doesn’t matter. One concern per PR. Refactoring and tech debt are separate concerns.
CC BY-NC-SA 4.0
You should refactor as needed as you go because refactoring cases are never gonna be prioritised.
Not with that attitude they won’t 😛
Refactoring in PRs just makes it more difficult to review. “Do these lines belong to the goal nor not?”. Also, we’re human and miss things. Adding more text to review means the chance of missing something increases.
Especially if the refactored code isn’t just refactored but modified, things are very easy to miss. Move an entire block of code from one file to another and make changes within = asking for trouble or a “LGTM” without any actual consideration. It makes code reviews more difficult, error-prone, and annoying.
Code reviews aren’t there to just tick off a box. They are there to ensure what’s on the tin is actually in it and whether it was done well.
CC BY-NC-SA 4.0
In my experience I haven’t had an issue because usually the refactorings are small. If they’re not I just hop on a call with the person who wrote the MR and ask them to walk me through it.
In theory I’d like to have time to dedicate solely to code health, but that’s not quite the situation in basically any team I’ve been in.
I haven’t had any trouble separating refactors PRs from ticket PRs. Make the ticket PR, make a refactor PR on that ticket PR, merge the ticket PR, rebase refactor PR on master, open ticket PR for review, done 🤷
CC BY-NC-SA 4.0