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.
I feel personally attacked.
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.
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.
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 🤷
As long as there’s less code than there was before, I’ll approve it
Deletes codebase
Looks about right, approved ✅
Bugs are all gone, boss
I laughed so hard at this, thanks
There was a guy I worked with that tended to want to unilaterally make sweeping changes.
Like, “we need the user to be able to enter their location” -> “cool. Done. I also switched the dependency manager from pip to poetry”.
Only a little bit of exaggeration
Some people, like me, are not built to be developers. I can sculpt code in any language I need for whatever problem I need to solve, but maintaining code over a long period of time, with others, is not my thing.
The drive to do additional changes is just too high and the tendency for typos or obvious logic errors is too common. (There is one little improvement. It’s right there. One line up. Just change it now while you are in there…)
I am not stupid and regard myself as a decent engineer but my brain is just wired in a more chaotic way. For some things that is ok. For developing code on a team, not so much.
Security is the field I am most comfortable with because it allows for creative chaos. Rule breaking is encouraged. “Scripting” is much more applicable and temporary.
When using git and are working on a feature, and suddenly want to work on something else, you can use
git stashso git remembers your changes and is able to restore them when you are done. There is alsogit add -pthis allows you to stage only certain lines of a file, this allows you to keep commits to a single feature if you already did another change that you didn’t commit (this is kind of error prone, since you have to make sure that the commit includes exactly the things that you want it to include, so this solution should be avoided). But the easiest way is when you get the feeling that you have completed a certain task towards your goal and that you can move on to another task, to commit. But if you fail you can also change the history in git, so if you haven’t pushed yet, you can move the commits around or, if you really need to, edit past commits and break them into multiple.Instructions unclear. Stash is 35 tall and I’m scared to look at what’s been fermenting at the bottom.
I mean, Poetry is a lot better then Pip. The only issue I see is that they broke some CICD stuffs farther up the chain.
It could be!
But part of working as a professional on a team is communicating and achieving consensus. Just trying to make a change like that out of the blue is poor form.
Also consider the opportunity cost: we had planned on getting XYZ done this week, and instead he spent a few hours on this and dragged a few people into a “do we want to change to poetry right now?” conversation
