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?
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”
Refactors should be a natural part of development or you will have a shit code base
Sure, now imagine you’ve been obligated to adopt a legacy codebase.
Life isn’t a classroom.
That’s pretty much all I have been doing in my 8 year career
LGTM
Let’s test in prod
Real men test in prod
Reaaal men of geeenius
🚢🚢🚢
Lol go try merge
This response is so true and so sad.
[Open]
“LTGM!”
- Last update a year ago
Better than “rejected - git gud”? :-P
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.
Or auto rejected when the format doesn’t fit.
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.
I’ve used pre-commit pretty extensively over the years and I’m confused.
Each linter has to be in its own repo (for most linter types). So it’s not really usable for project-specific lints.
Not sure what you mean by this. I have pre-commit set up to do linting in several different projects, and even have it running multiple differently-configured lint jobs in the same repo.
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.
Again, I have pre-commit set up on multiple repos running pylint with multiple different plugins. Pre-commit absolutely does take care of setting up venvs with needed dependencies.
Not sure what you mean by this. I have pre-commit set up to do linting in several different projects, and even have it running multiple differently-configured lint jobs in the same repo.
I don’t mean using lints, I mean writing custom ones. Say you have a custom lint you want to use but it only will ever be used for that specific project. You can’t just put the lint code in a subdirectory. It has to go in a separate repo.
Pre-commit absolutely does take care of setting up venvs with needed dependencies.
Again I think you might be misunderstanding. It will install pylint fine, but if your project does e.g.
import yaml, it’s not going to set up a venv and install pyyaml for you.Say you have a custom lint you want to use but it only will ever be used for that specific project. You can’t just put the lint code in a subdirectory. It has to go in a separate repo.
You can run locally defined hooks with pre-commit, just define them in the
repo: localsection of the.pre-commit-config.yaml, and have it run a bash/python/whatever script or something that invokes your custom linting, wherever it lives in your file structure.It will install pylint fine, but if your project does e.g.
import yaml, it’s not going to set up a venv and install pyyaml for you.Yeah I misspoke/misremembered there. For Python based stuff, it uses the currently active virtualenv or your global python install, so it relies on you installing your own dependencies. Which isn’t really that big a deal imo, because you need to install those dependencies to run/debug/test locally anyways.
You can run locally defined hooks with pre-commit, just define them in the repo: local section of the .pre-commit-config.yaml
Sounds like you’re just googling it rather than actually speaking from experience. Suppose I have written a Python lint and it’s in my
ci/lints/foofolder. How do I tell pre-commit that? (Hint: you can’t)Which isn’t really that big a deal imo
For small Python projects, maybe not. The project I’m working on has multiple sub-projects and those each have their own venvs, pyproject.tomls, etc.
-
deleted by creator
Some diff tools don’t handle indentation by default.
So if you add a wrapper, it counts everything inside it as “changed”
That’s what “toggle whitespace diff” is for.
You can do that? How?
Pre-commit hooks is a common approach to this, so that whatever is committed gets processed. Another possibility would be to set a bot on the repo to do automated commits after human-made ones, but that can get a little noisy.
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”.)
That’s when you set the intern’s IDE to preserve the line endings.
.gitattributes is our best friend
Automatic code formatter with company style rules for more consistency across all developers.
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.
Now that’s just crazy talk.
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.
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.
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.
It’s even more cyclical. I usually can’t remember the reasons why I made the change to begin with.
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
Which one of you loose cannons down voted this?
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.
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…”
Last time I got this PR I was like, “Okay, I’ll do my best, but you asked the guy that has like 30 mins a day to actually focus and look at someone else’s code AND yours isn’t the only PR I’ll have to look at this sprint. Have fun reminding me about this for the next week.”
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.
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.
True, but at the same time its very possible to go too small. A bunch of one line code reviews can really slow progress easily.
Stuffing multiple tasks into one PR is often bad.
- It’s harder to review. As a reviewer it’s difficult to know which code change is related to which task.
- It’s harder to verify. Did you really test every change you made?
- You might end up with a “hostage” situation. There might be a few code changes in the PR that looks good and is really wanted, but other code changes in the same PR of lower quality. As a reviewer, should you just let these lower quality code changes slide so you can bring in the code change you really want? Probably not, but you’re going to let it slide either way.
You might end up with a “hostage” situation.
Reminds me of a guy I used to work with.
He’d have like a pretty normal PR to do something like change a field to be read only in some API. But then snuck into the pr there’d also be something like “change application to run locally different than prod for entirely selfish reasons” or “lower global coverage requirements” or “disable type checking in this whole folder”
I also worked with a guy like that. Impossible to predict what he was going to do in his next PR. Always a nightmare to review. Also exhausting to argue with, so let some things slide because I was so done dealing with his bs.
This guy was also difficult to argue with. He was always professional, which I guess is worth something.
One time he tried to remove all the types from a bunch of code “because they were all going to change later.” I refused to allow this. We went back and forth in the comments for hours. I think eventually the lead or boss got involved. Thankfully people sided with me.
His “later” project that was allegedly going to change all the things was rejected by the team. The code in question is still used and properly typed a year later.
Right but it’s pretty rare that a tiny PR actually accomplishes a valuable user story.
So my point is just that lines of code is mostly irrelevant as long as it’s organized well and does no more than necessary to accomplish the agreed upon goal.
What anime this from?
Watashi ni Tenshi Ga Maiorita
https://myanimelist.net/anime/37993/Watashi_ni_Tenshi_ga_Maiorita?cat=anime
My first PR at my current job was about 130 files for the front-end, and about 70 for the backend. This hits close to home.
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.
