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.
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.
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 🤷
Fair enough
A tiny bit of value, but definitely not worth the pain and effort. It wasn’t exactly any technical debt that hindered our development.
We had other places with way more pressing technical debt that could’ve been focused on instead.
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”
LGTM
Let’s test in prod
Real men test in prod
Reaaal men of geeenius
🚢🚢🚢
Lol go try merge
sets the diff to ignore whitespace
Lines changed: 3
Haha! Jokes on you! It was mostly gnu makefile calls to ruby scripts!!! You’ve just broken the build a million different ways!
Joke’s* on you
(Short for “The joke is on you”.)
The pipeline should handle formatting. No matter how you screw it up, once you commit, it gets formatted to an agreed upon standard.
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 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.
Only 35? That’s rookie numbers.
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.
Do you have ADHD?
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.
Thank you so much for your reply. Your comment was so recognizable to me, and I’m in the process of being diagnosed with ADHD.
Edit: I want to say that I do feel sorry for asking such a personal thing about you. I’m not young either, but just now coming to terms with it and figuring out that this is the reason why everything I do goes as it does. To recognize it in the wild is absolutely weird.
No worries! I am generally very open about it. (Your comment was recognizable to me, actually.) There is a very specific non-malicious bluntness that comes with the condition, actually.
But yeah, you have been practicing dealing with it your entire life. Treatment just helps a ton.
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.
It’s even more cyclical. I usually can’t remember the reasons why I made the change to begin with.
At least there are more removals than additions.
Jokes on you that’s just the README being deleted since it no longer matches the code.
Last time somebody did this to me there were a lot of sit downs about how to properly chop up large scale code changes and why we don’t sit on our own branch for two months.
“How long will this take to get in?”
“Well, two weeks for me to initially review it, a week for you to address all the changes, then another week or so for me to re-review it… Then of course we have to merge in all the changes that have been happening in primary…”
My team lead: “I’ll 🙈 review”
Net removal of 1500 LoC…
I’m gonna make you break this up into multiple PRs before reviewing, but honestly, if your refactoring reduced the surface area by 20% I’m a happy man.
it’s just removed unit tests that didn’t work any more…
😱
Please, no, I get flashbacks from my 6-month journey (still ongoing…) of the code review process I caused/did. Keeping PR scope contained and small is hard.
From this experience, I wish GitLab had a “Draft of Draft” to tell the reviewer what the quality of the pushed code is at: “NAK”, “It maybe compiles”, “The logic is broken” and “Missing 50% of the code”, “This should be split into N PRs”. This would allow openly co-develop, discuss, and steer the design, before moving to nitpicking on the naming, formatting, and/or documentation details of the code, which is likely to drastically change. Drafts do work for this, but the discussions can get uncomfortably long and convolute the actual finishing of the review process.
Once both reviewer(s) and the author agree on the code design, the “DraftDraft” could be collapsed into a link in an normal Draft to be mocked next. The scope of such draft would be limited by the earlier “DraftDraft”.
lgtm
This does seem like a potential issue if the PR is itself implementing more than one vertical slice of a feature. Then it could have been smaller and there might be wasted effort.
If the patches are small and well-organized then this isn’t necessarily a bad thing. It will take more than one day to review it, but it clearly took much more time to write it.
What anime this from?
Watashi ni Tenshi Ga Maiorita
https://myanimelist.net/anime/37993/Watashi_ni_Tenshi_ga_Maiorita?cat=anime
I try to keep my changes under 300-350 lines. Seems like a good threshold.
I’m still annoyed that Github doesn’t have good support for stacked diffs. It’s still not possible to say that one PR depends on a different one, and still has no ability to review and land them as a stack.
How is this different from creating a feature branch and making your PR against them until everything is done, then merging that into the main branch?
also iirc gitlab does offer something like this as a feature now with “merge trains” (though i’ve never really used it, usualy just go for the feature branch out of habit x) )
Making PRs against a feature branch still has the same problem.
Say you’re working on a major new feature, like adding a new log in flow. It’s a good idea to split it into many small changes: create initial log in form, implement user sessions, add SSO support, add “remember me” functionality, etc.
Those changes will likely depend on each other, for example adding the “remember me” checkbox requires the form to actually be built, and you probably don’t want to wait for the reviewers to review one change before continuing your work. This means you naturally end up with PRs that depend on each other in a chain.
Stacked PRs (or stacked diffs, stacked MRs, whatever your company calls it) means that:
- The code review tool lets you specify dependencies between the PRs, for example the “remember me” PR depends on the initial login form implementation
- It shows the dependencies visually in the UI, like a chain or tree
- Changes can’t be landed until the PRs they depend on have been reviewed
- There’s a way to land an entire stack
- When you review a PR later in the stack, it doesn’t show any of the changes that were made earlier in the stack. Each PR focuses just on the changes in that part.
- For each PR, the build steps and linters run for all the changes in the stack up until that point. It helps detect if incompatible changes are made earlier in the stack.
Making all your commits directly to a branch then creating a PR for the whole branch is similar, but reviews are a nightmare since it’s only a single review for the entire branch, which can be thousands of lines of code.
At my workplace, we use feature flags (essentially
ifstatements that can be toggled on or off) rather than feature branches. Everyone works directly from the main branch. We use continuous deployment which means landed code is quickly pushed to production. New features are hidden behind a feature flag until they’re ready to roll out.You can make a PR against your feature branch and have that reviewed. Then the final PR against your man branch is indeed huge, but all the changes have already been reviewed, so it’s just LGTM and merge that bad boy!
You can make a PR against your feature branch and have that reviewed
But what if you have multiple PRs that depend on each other? Or are you saying only have one PR open at a time? That sounds like it’d be very slow?
I suppose it is possible to have two PR that have changes that depend on each other. In general this just requires refactoring… typically making a third PR removing the circular dependency.
It sounds like your policy is to keep PR around a long time, maybe? Generally we try to have ours merged within a few days, before bitrot sets in.
Sorry, my comment was unclear. I didn’t mean a circular dependency, just PRs that have a chain of dependencies (e.g. PR 100 that depends on 99, that depends on 98, that depends on 97)
They’re usually not around for a long time, but there can be relatively large chains if someone is quickly adding new features.
Keep changes small, we use git patch stack https://github.com/uptech/git-ps
Human made changes is likely not what caused this image to occur.
111 files with that kind of change count is most likely a dependency update. But could also be that somebody screwed up a merge step somewhere.
The only way I see that is a dependency update is if you’re versioning your node_modules or <insert-folder-here> which is generally a no-no
Many organizations vendor packages in the repo for a number of different reasons and languages. Not just for node.
Or maybe their IDE had a different auto indent config and they saved it all, then committed it all without checking the
diffor thestatus.You should have an agreed upon format that is enforced by cicd. Prettier, black, whatever.
I do like the idea of mandating
git clang-formatas the Kate project has.
That way the other devs don’t need to change their own IDE settings to comply.
I’m still annoyed that Github doesn’t have good support for stacked diffs. It’s still not possible to say that one PR depends on a different one, and still has no ability to review and land them as a stack.
That’s when you set the intern’s IDE to preserve the line endings.
