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.
this post was submitted on 29 Mar 2024
965 points (98.3% liked)
Programmer Humor
23527 readers
1744 users here now
Welcome to Programmer Humor!
This is a place where you can post jokes, memes, humor, etc. related to programming!
For sharing awful code theres also Programming Horror.
Rules
- Keep content in english
- No advertisements
- Posts must be related to programming or programmer topics
founded 2 years ago
MODERATORS
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.
load more comments
(1 replies)
You should refactor as needed as you go because refactoring cases are never gonna be prioritised.
load more comments
(4 replies)
load more comments
(1 replies)
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.
load more comments
(1 replies)
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.
load more comments
(6 replies)
load more comments
(4 replies)
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.
As long as there's less code than there was before, I'll approve it
Deletes codebase
Looks about right, approved ✅
load more comments
(1 replies)
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..."
load more comments
(1 replies)
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.
load more comments
(2 replies)
lgtm
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".
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.
load more comments
(1 replies)
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?
load more comments
(6 replies)
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.
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.
you should meet my coworker. this is one week worth of work. and he still only commit once a week.
load more comments
(2 replies)
load more comments
(3 replies)
load more comments
(1 replies)
Love it when my coworkers reformat the code style, making it nigh impossible to understand what they actually changed, while greatly inflating their “contribution.”
It also blows away the git blame, making it hard to know who actually changed that one critical line of business logic 3 years ago that you need to understand before trying to fix some obscure bug.
I have one coworker who does this constantly and if you just looked at git blame, you’d think he wrote the entire code base himself.
load more comments
(1 replies)