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.
Doesn’t matter. One concern per PR. Refactoring and tech debt are separate concerns.
Or, if the team does allow refactoring as part of an unrelated PR, have clean commits that allow me to review what you did in logical steps.
If that’s not how you worked on the change than you either rewrite the history to make it look like you did or you’ll have to start over.
You should refactor as needed as you go because refactoring cases are never gonna be prioritised.
I hear you, but they should MVP the ticket, close it, then concisely collar the PM/lead and say “I know how to make this better and am hungry to do it. Let me address some tech debt next sprint. I got this and will keep it timeboxxed. I’ll also ensure my changes pass QA before coming to you”
Refactors should be a natural part of development or you will have a shit code base
LGTM
sets the diff to ignore whitespace
Lines changed: 3
The pipeline should handle formatting. No matter how you screw it up, once you commit, it gets formatted to an agreed upon standard.
Yeah I think that’s what he meant. You don’t want CI editing commits.
I use pre-commit for this. It’s pretty decent. The major flaws I’ve found with it:
-
Each linter has to be in its own repo (for most linter types). So it’s not really usable for project-specific lints.
-
Doesn’t really work with e.g. pyright or pylint unless you use no third party dependencies because you need a venv set up with your dependencies installed and pre-commit (fairly reasonably) doesn’t take care of that.
Overall it’s good, with some flaws, but there’s nothing better available so you should definitely use it.
Some diff tools don’t handle indentation by default.
So if you add a wrapper, it counts everything inside it as “changed”
Haha! Jokes on you! It was mostly gnu makefile calls to ruby scripts!!! You’ve just broken the build a million different ways!
That’s when you set the intern’s IDE to preserve the line endings.
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.
Make those changes in an own commit, or keep it to files you already have to touch.
When using git and are working on a feature, and suddenly want to work on something else, you can use git stash so git remembers your changes and is able to restore them when you are done. There is also git add -p this 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 do. Also, I am old(ish) so I have had many years to come to terms with what I can do well and where I struggle.
In this case, I didn’t want to use it as a crutch or an excuse. After reading the number of awesome replies this morning, I realized I should have probably framed my comment differently.
People here put some real time and effort into giving some solid advice and I didn’t expect that.
Edit: As a pure example, this is the third or fourth edit of this comment. Words are challenging, and programming is very similar in that regard.
I tell my young developers - the primary goal in software engineering is maintainability. Code reuse, encapsulation, abstraction, and myriad other concepts all contribute to ease of maintaining source code over the long term. Maintainability allows for easier, predictable feature addition and removal, with fewer changes, and by different people. You’re also a different person than the one you were months or years ago when it comes to software.
Did I already post in here about how he wrote a custom DSL instead of using the standard widely used ORM we use everywhere? Maintainability nightmare.
He doesn’t work here anymore and now I have to either figure it out or rip it out. So far it looks like “rip it out” because it took less than an hour to swap in the orm, and now there’s just a lot less code needed.
Very relatable. Especially when it’s less effort to make the change than it is to try and ignore it. But it’s understandably harder for those who are reviewing your work.
I’m the same way. Chasing code changes through the codebase then fighting with an edit rebase stack trying to explode them into less-interlocked changes.
It doesn’t always work, sometimes I am just committing a giant blob of changes at work on my project I near-solo maintain 💀
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
That wasn’t me, but that also used to be me. I learned to pick my battles, especially with complex code bases, and tried to keep scope creep in the name of improvement to like a dozen lines (provided it was fully tested).
I think it’s definitely a thing most people grow out of when they gain experience.
My boss told me about how when he was new he rewrote a whole chunk of the front end. His boss gave him a talking to about how you can’t just go and do that when you’re working with a team.
At an old job I just opened a PR to apply a code formatter to an internal project I wasn’t even a routine contributor to. PR was rejected and I learned a valuable lesson about talking and getting buy-in before making sweeping changes.
