ProgrammerHumor
damnItsTime
Discussion
Not the same if 13000 lines of C or an updated lock file… Context matters.
It is a lot of whitespace with some added features sprinkled in between.
Reject that.
Whitespace changes and added features should be separate commits. They can be in the same PR, but not the same commit.
(Unless use squash commits. Then they need to be separate PRs too.)
Yet another reason why squash on merge is stupid. That's a hill I will die on
At least someone wants to contribute haha no, good luck with that
If that's anything other than "used autoformatter to fix whitespace" or "optimised SVG files", then rejected for being too large
Or, maybe in some cases, auto-generated definition files or some such (although often that'd make more sense to be part of build than checked in)
tip: for formatting prs, use .git-blame-ignore-revs so git blame does not blame whoever did that formatting commit, but whoever last changed the actual code
Thanks! Git blame gets used a LOT at my job, so this'll come in handy.
Why? I can imagine a lot of bad reasons and not many good ones. I feel like git blame should be sparsely used.
Using it to assign blame isn't useful, but using it to see what PR made a change and (hopefully, if it's well documented) why it was changed can be helpful
Yeah it's really useful to get a bit of context behind stuff on a few slack pings.
I am currently working on my resume to leave the company. This is one of the biggest reasons.
That's one hell of a tip. I didn't know about this feature, thanks a lot!
At my job we just refer to an empty folder called "doc"
Huh, I wonder if Perforce has something like this.
Testdata for unit tests has been the cause of my biggest PRs.
Nice guess, you are correct!
Ooh, on which one? Autoformatter?
At least that should be simple enough to review - run the autoformatter yourself (because they also checked in autoformatter config, right?) and see if it's the same
Yeah about that... We are not yet using autoformatters. I tried to explain it to my boss, but he did not want me to spend time setting it up. We do have a readme in which all conventions are mentioned...
Rookie mistake, you don't ask about shit like that.
I didn't. I got fed up with my boss' feedback on my code. (Biggest one was not being allowed to use a switch statement.) So, naturally, I had a list of all remarks that I found unlogical so my code wouldn't be rejected. He found out about the list, got real mad at me for a day. Then, he introduced a "code book" in which are basically naming conventions and limits on what features to use.
I'm cleaning up my resume tonight, the final straw was not being promoted after a year. I make 2200 a month gross based in the netherlands. (32 hours)
oh man.. I'm over like "wtf is a seitch statement", thinking it must be something really cool I've never heard of.. embarrassing
My bad, I meant switch
yeahh, I know.. and it should have been obvious.. but I am still enjoying the first sips of my morning coffee and needed google to hit me with "did you mean switch statement" first
It sounds like he just introduced standards.
All of those things that he introduced should be introduced.
Yes I agree that standards should be used. I do not think that those standards should prohibit basic features of a programming language.
I guess limit features is quite wide in interpretation.
Anything that was particularly egregious?
A ban on the .map function, no forEach loops and no switch statements. Because of unreadability.
What about my 65 thousand lines of code though.
On our company we do format check at pipeline level so it fails. You are forced to always format it so it is always up to date
Yeah, I’m struggling to see how there could be 13k+ deleted lines that were inconsequential. I guess if you have a mammoth code base and you just lint-fix the shit out of everything.
For me, once, the junior told me “The tests broke after my change so I deleted them”
I think your quotes are in the wrong place, surely that should be
> the junior told me "The tests broke after my change", so I deleted them
😆
Same with package.lock
Maybe...
If it were +12.5k -13k then maybe it's just new minor/patch versions of a lot of dependencies, but the changes are too asymmetrical for that - you're pretty much never going to get that with just updating deps to a new minor/patch version
If you have +2.5k -13k from package lock, then you've got a lot of dependency changes, and it needs a lot of inspection
Maybe you've updated to a new major version of a direct dependency and that's resulted in far fewer packages required, but I'd be suspecting there were multiple dependencies updated with that large a change (and each direct dependency update to a new major version should be its own merge request)
My autoformatter of choice is the find/replace in notepad++
Find four spaces, replace with \t
People have really screwed up the grammar of this meme. It should be either "It is my great disappointment to inform you" OR "It is with great disappointment that I inform you", not a mashup of the two.
Don't look at the singular gentleman.
Just decline it and tell them to break it down into more granular PRs
Ok I will review asap after my feature is done, that’s really soon (the next milestone)
The initial developper: « It is just adding plurals in the wording »
Edit: Typo
I'm preparing one MR to send before I take my vacation.
Right now it's +13000 -15000
This is very simple: the PR is too big. Please split changes into smaller PRs
Do two or three random and generic comments, then approve and hope someone else has to deal with the consequences
LGTM
lgtm
Just reviewed one that was +2.4M, -800k. Fortunately it was almost entirely generated files, so I only really had to review the generator code itself. Still pretty daunting when you first see it.
Hot damn.
Well, at least they were kind enough to inform you that it is a trap beforehand.
Though, does that make it not a trap...?
LGTM... Deploy it
When you realize your career now includes babysitting code, not just writing it.
Also, playing policeman and forcing people to enforce guidelines and the likes, until one stubborn junior thinks he too good for it and find yourself triggering a PIP after repeated offence
What's a PIP?
Performance Improvement Plan. In theory, it's an agreement to correct how you're performing your job. If you don't shape up, you're fired. In reality, it's usually just a CYA by the employer to say "well, we tried" before firing an underperforming employee.
A process you have imposed on you by your supervisor because of bad performance, often resulting in a termination of contract (it wasn't always like this, but today it is a bad omen)
Thanks
Short for performance improvement plan, if I'm correct. Ironic, if so, as it seems to be interpreted as a step before termination by many
Sure thing
Performance Improvement Plan, sugar coat and a severance waiver so that employer can sack you without any reason, deliverables are often unrealistic to begin with
It's a death sentence, once you get one bail and go to the next job and survive until you get one there
I mean, I knew this was goin to happen at some point. Just expressing my disappointment.
Bruh, who the hell submitted a school essay as PR
One of the hardest things to find out. Your company thinks your more efficient doing reviews than coding. The worst part is they are probably right.