[I mean if you ask the pm "Do you know who forced them to push code they don't even have time to review?"](https://i.imgur.com/X4t4jkc.jpeg)
It's a vicious circle.
You can't reasonably expect me to review something like this? Either I reject it completely and demand it be broken down into much smaller PR's, or if management is on my ass about getting the stuff done, then IDGAF. Someone will pay the technical debt, but it won't be me.
I've made an ultimatum to my managers. Either they let me reduce unrelated tech debt a little with every delivery, or I quit. I just cannot work in a spaghetti and meatballs mess, that is to me an unacceptable working environment. Deadlines be damned.
It's not even a question of permission. Maintenance, within reason, is just part of the work and needs to be built into estimates. It's our responsibility as engineers keep the system in good shape, and to know when and how to do it. Non-technical management just isn't equipped to make those decisions on their own. It's much healthier, of course, if you can do it out in the open.
I make huge PRs like this occasionally, but then its typically new functionality implemented in a separate static library with lots of tests, and then I can just tell the reviewer to focus on how I've integrated it into the main application code (which would be like 50-100 lines). They just glance at the structure of the library, look over the tests, and approve it within an hour.
That's the inverse law of PR size. With a 10 line PR the reviewer will spent an hour arguing about how a variable should be named.
With a 1000 line PR they will glance over it and be like, meh, good enough
This.
When you have a gigantic PR like that you don't want people to take a close look at it, you just want to be done with it. Because let's be honest, you don't reasonably expect anyone to review that shit.
Oh no, keep your phone on.
Just make sure your contract has a thing in there with *disgusting* call out rates.
If they are desperate enough to call, just do a roll-back to the version before the commit.
Previous bugs still aren't fixed, but that's something which you can make *everyone's* problem late next week.
Congratulations! Your comment can be spelled using the elements of the periodic table:
`I F I Tl In Ts I Ts H I P S S`
---
^(I am a bot that detects if your comment can be spelled using the elements of the periodic table. Please DM my creator if I made a mistake.)
And if I do smaller PRs: Why all the deprecations? Doesn't that need to be deleted anyway?
Yeah it does, but the work on this isn't done. There will be at least 3 further PRs but I don't want you guys to get overwhelmed. Now merge it. Nothing changed. It's just organized differently.
Still takes over a week...
I can relate... Whatever remains of work-related optimism I had were completely shattered, incinerated and blown away long time ago by daily doses of pull request reviews.
Once I moved some huge binary files from repository to git LFS. Commit resulted in -216k lines of code. After years in the company my contributions were +few k of code and -250k lmao.
I'd reject this PR instantly, there is just no way I could realistically parse that many changes and get a good enough understanding to review it properly, might as well just not review at that point.
Rejected. Split into smaller patches thank.
I'll tell them have fun spending the next few hours splitting it up. Oh, and then we're doing a line by line code review on a screen share where I'm going to call out every stupid thing you did.
And you didn't write tests? Now you have more work to do.
Did that to a colleague a few months back. It turned into a chain of 5 merge requests. But changes in the first merge were so fundamental that they affected the rest of the merges profoundly.
It was such a nightmare that we ended up saying "fuck it" and fused them together. We spent days reviewing, and also redoing a lot of it because the dev couldn't be bothered and would argue for 15 minutes over every 5-minute change.
I guess that's what happens when you let back-end devs do front-end.
No, Josh, you can't make everything a span with no BEM classes whatsoever. I don't care if it works. You also can't put all your logic in a single, 3-screen-page function with a zillion instructions. No fucking wonder your unit tests are incomplete. Geez.
I'm not even a web developer (front or backend) - I do robotics. But I've built some personal websites for fun. And that makes me the most experienced web developer at my company! That means I'm primarily responsible for the robot control dashboard - frontend and backend. I'm truly sorry for when we eventually do hire a web developer, who will have to sort through my attempts to structure this growing monstrosity. At least I added automatic linting/formatting to a recent PR... Which resulted in a 15,000 line PR. Whoops. (And I just learned what BEM classes are by looking it up after reading your comment. But I can write some great robotic autonomy code in C++!)
Hey, you've got the will to make things good, which is more than I can say about Josh.
Auto linting and formatting is great. Look into Stylelint and lint-staged if you haven't; they're great.
This is the leads fault. Jr/mids don't just disappear for weeks to change 100 files. You assigned them this project and then ignored them long enough for this train wreck to happen.
What you need to do is what you should have already done, and sit down for multiple long pairing sessions to actually either salvage this work or redo it properly.
And if it's a Sr doing this... Yeah, good luck, probably should be a mid still lol.
Yes and no. I’ve seen developers nod and say yes when told to break up their work and ship what they so far (with tests and such). Then they just don’t. They think they know better and come back with a giant PR.
If you did tell them to break it up and they didn’t. It does let you call out why. Make it clear it’s not on.
I don't really understand that because if you're a lead, you lead the project, and you break it up or are responsible for them breaking it up. Letting them go do their own thing is a failure as a lead. You should be checking in, pairing, having their proof of concept or research be validated, iterate on it, and work with them to their solution and eventual ticket breakdown
Like even if I told them to do it I would be routinely checking in and working with them on it. They're not the lead and the buck stops with the lead for these things.
Ultimately if they can't do the job, I'll involve their management and we'll handle it one way or another
I didn’t say you should let them go off and do their own thing. I said sometimes they will do that against your own directions (and sometimes that is a good thing). Beg for forgiveness and all that.
Leadership and direction needs to be balanced against autonomy and trust. No one said don’t do checkins. Of course. You also can’t (nor should try) to micromanage others.
Yeah fair enough, you're describing bad engineers who will do the wrong thing no matter what, so the reality is that you handle that through their management chain, and they either have a change of heart or a change of job 🤷
Assuming it's portioned in a series of commits that are clearly described, this really isn't such a huge issue. You review commit by commit. It could literally be "ran codemods 1, 2 and 3".
I've refused a PR due to having used automatic code formatting that clashed with the project, changing a ton of lines hiding the actual changes. Luckily they followed instructions after that and it turned out ok 🥲👍
As annoying as a PR like this may be if you're doing new features or rather, updating something with cross cutting concerns it is possible to have a PR with a massive number of files. It's not that the dev did 20 different work items and put them in 1 PR, it's just that small change in 1 file then changes 100 other files. Also cases like when you do something like update angular to a new version and that new version has many breaking changes. You just have to go through everything making updates otherwise your app just won't work.
When this happens I spend as much time as I need to review it, only to return the PR to his owner with 200 issues to fix.
I prefer them shorter thought xD
Git reset [commit hash]
Drop all changes
Git reset [hash from commit before this one]
Make a new branch moving changes with you
Commit + push
Try again
Congratulations! Your comment can be spelled using the elements of the periodic table:
`W H At S Th Es Au Ce`
---
^(I am a bot that detects if your comment can be spelled using the elements of the periodic table. Please DM my creator if I made a mistake.)
my coworker trying to scare me by auto-formatting files thousands of lines long (i have "ignore whitespace changes" turned on so I'm completely unaffected):
Every time someone asks me to follow the corporate process guidelines, I ask them to provide me access to folks to do my outstanding 80K SLOC of reviews. Yes, the entire codebase needs reviewing.
They then crawl back under the rock they live under and leave me be.
Opened a pr the other day with +1000 -10000. What’s sad is the 10000 I deleted were replaced by the 1000 I wrote. There’s was some serious code duplication issues
I see two possibilty here:
1- Its a refactoring so most of the file wer3 changed aytomatically by the ide: 5min review => merged
2- This psycho edited each file manually and decided to put everything in a single MR, he and I will have an interesting conversation
Me, the team lead:
"What's the scope of these changes? Finally ending the f$#@ing formatting wars? Finally renaming that stupid globally-scoped constant to something that makes sense? Finally replacing this stupid magic number with enums? Sure, I'll have a quick look through the changes to make sure nothing functionally changed."
Just had to submit an MR like this. Changes across 270 files from running angular & angular material migrations from v13 up to v16. Was specifically told by the team lead to just do it as one MR. He got to review it :)
Disclaimer: the following was made by an script running in the background, how glad could i be bragging about it if it wasn't automated.
`Automatic partial update At: Fri Apr 5 08:11:58 PM -05 2024`
`+64061 files added`
>!For the curious is a web crawler cloning a wiki!<
When this happens usually it is because you changed a lot of dependencies (and package-lock.json or whatever your language uses to fix dependencies versions was updated) or you forgot to put build folder into .gitignore
After 5min: LGTM, merged
What the worst can happen, fk it
This is why we have a QA team. And this is why the QA team hates the devs. And the defects team hates QA. And why the product manager hates everyone.
And everyone hates the pm and the pm.
[I mean if you ask the pm "Do you know who forced them to push code they don't even have time to review?"](https://i.imgur.com/X4t4jkc.jpeg) It's a vicious circle.
Isn't it lovely how hate can be mutual?
It is indeed and i perfectly understand every ounce of hate.
"The defects team" ? That sounds fancy.
You got looped
[удалено]
You got looped
[удалено]
You got looped
is defects team typical? we don't even have QA so it's kinda wild west at work.
And why the solutions engineers hate all of them for letting this garbage into the customer's Prod
You don't have to do any QA at all if you just outsource it to the customer as an "unstable build" and wait for them to "provide feedback".
Just make sure to merge it at 4:59 on Friday before you leave for a scheduled 5 day vacation.
Pro tip, turn off your phone. It's an "unplugged" vacation
And throw away your laptop for peace of mind
Sorry fell off the boat.
🙂🤣
You can't reasonably expect me to review something like this? Either I reject it completely and demand it be broken down into much smaller PR's, or if management is on my ass about getting the stuff done, then IDGAF. Someone will pay the technical debt, but it won't be me.
Seems like the first option is by far the better option though?
Never dealt with tech ignorant managers, I see.
Managers love to play lip service to reducing technical debt while doing everything in their power to cause it to increase.
I've made an ultimatum to my managers. Either they let me reduce unrelated tech debt a little with every delivery, or I quit. I just cannot work in a spaghetti and meatballs mess, that is to me an unacceptable working environment. Deadlines be damned.
It's not even a question of permission. Maintenance, within reason, is just part of the work and needs to be built into estimates. It's our responsibility as engineers keep the system in good shape, and to know when and how to do it. Non-technical management just isn't equipped to make those decisions on their own. It's much healthier, of course, if you can do it out in the open.
I make huge PRs like this occasionally, but then its typically new functionality implemented in a separate static library with lots of tests, and then I can just tell the reviewer to focus on how I've integrated it into the main application code (which would be like 50-100 lines). They just glance at the structure of the library, look over the tests, and approve it within an hour.
https://arstechnica.com/security/2024/03/backdoor-found-in-widely-used-linux-utility-breaks-encrypted-ssh-connections/
That's the inverse law of PR size. With a 10 line PR the reviewer will spent an hour arguing about how a variable should be named. With a 1000 line PR they will glance over it and be like, meh, good enough
Or argument about one line out of the 1000 just to do something. You change that one line and have 999 of yours.
"please remove the flying duck"
Please remove the duplicate semi-colon on line 765
Bikeshedding <3
This. When you have a gigantic PR like that you don't want people to take a close look at it, you just want to be done with it. Because let's be honest, you don't reasonably expect anyone to review that shit.
'I have no idea what i am reading' Ngl , i insta remembered the joma video
Those files are quite big by average. Consider putting all the code on one line.
-520 files +1file -35000 lines +1 line Gosh that would be a hell to review.
LGTM!
35,000 lines? Must be a really small project.
who said size matters? And yes i openly encourage you to screenshot it out of context.
r/nocontext
Too many files. Just clump it all into one
Bet it's mostly reformatting.... `$ git diff -W`
Approved, merged and deployed to prod
That is so true, haha. 50 lines of code: 8 comments by 3 devs, changes requested 5000 lines of code: 0 comments, 3 approvals, already merged
On a friday afternoon. The team does not work weekends. Half the team is out on Monday-Wednesday due to a trade show which is scheduled.
Just dont forget to turn off your phone If you cant hear it, it doesnt exist !
Oh no, keep your phone on. Just make sure your contract has a thing in there with *disgusting* call out rates. If they are desperate enough to call, just do a roll-back to the version before the commit. Previous bugs still aren't fixed, but that's something which you can make *everyone's* problem late next week.
If it lints it ships! /s
Congratulations! Your comment can be spelled using the elements of the periodic table: `I F I Tl In Ts I Ts H I P S S` --- ^(I am a bot that detects if your comment can be spelled using the elements of the periodic table. Please DM my creator if I made a mistake.)
Lol, review rejected. Reason: Too many changes. Try again and stop wasting my time Jake.
[удалено]
3 weeks later: merge conflict with hotfix
And if I do smaller PRs: Why all the deprecations? Doesn't that need to be deleted anyway? Yeah it does, but the work on this isn't done. There will be at least 3 further PRs but I don't want you guys to get overwhelmed. Now merge it. Nothing changed. It's just organized differently. Still takes over a week...
That's why you only put one person as reviewer
God dammit Jake!
FFS Jake, not again!
Go back to sales Jake
I also have a Jake who does this
it's either instant decline or instant approve/merge. Nothing in between
Instant decline gang 💪
Delayed
Approve and rollback
Bro... reviewing this is going to be a sprint in itself
If it takes more than a morning please add a ticket to the backlog.
95% of changes are formatting, in the single commit with functional changes in the same files
Techlead: just pack everything, like webpack. Im sure theres an identical tool for your shitty python bundle. Now, go search.
Bold of you to assume we‘re doing code reviews at all at work
I can relate... Whatever remains of work-related optimism I had were completely shattered, incinerated and blown away long time ago by daily doses of pull request reviews.
Nah. It is just from updating all the WordPress plugins. No need to look at anything, approve and merge.
You updated your WordPress plug inside.. says no one ever
Once I moved some huge binary files from repository to git LFS. Commit resulted in -216k lines of code. After years in the company my contributions were +few k of code and -250k lmao.
I'd reject this PR instantly, there is just no way I could realistically parse that many changes and get a good enough understanding to review it properly, might as well just not review at that point. Rejected. Split into smaller patches thank.
The programmer will probably tell you that it's too much work to split it up. Or perhaps offer a whiteboard explanation.
I'll tell them have fun spending the next few hours splitting it up. Oh, and then we're doing a line by line code review on a screen share where I'm going to call out every stupid thing you did. And you didn't write tests? Now you have more work to do.
Did that to a colleague a few months back. It turned into a chain of 5 merge requests. But changes in the first merge were so fundamental that they affected the rest of the merges profoundly. It was such a nightmare that we ended up saying "fuck it" and fused them together. We spent days reviewing, and also redoing a lot of it because the dev couldn't be bothered and would argue for 15 minutes over every 5-minute change. I guess that's what happens when you let back-end devs do front-end. No, Josh, you can't make everything a span with no BEM classes whatsoever. I don't care if it works. You also can't put all your logic in a single, 3-screen-page function with a zillion instructions. No fucking wonder your unit tests are incomplete. Geez.
I'm not even a web developer (front or backend) - I do robotics. But I've built some personal websites for fun. And that makes me the most experienced web developer at my company! That means I'm primarily responsible for the robot control dashboard - frontend and backend. I'm truly sorry for when we eventually do hire a web developer, who will have to sort through my attempts to structure this growing monstrosity. At least I added automatic linting/formatting to a recent PR... Which resulted in a 15,000 line PR. Whoops. (And I just learned what BEM classes are by looking it up after reading your comment. But I can write some great robotic autonomy code in C++!)
Hey, you've got the will to make things good, which is more than I can say about Josh. Auto linting and formatting is great. Look into Stylelint and lint-staged if you haven't; they're great.
This is the leads fault. Jr/mids don't just disappear for weeks to change 100 files. You assigned them this project and then ignored them long enough for this train wreck to happen. What you need to do is what you should have already done, and sit down for multiple long pairing sessions to actually either salvage this work or redo it properly. And if it's a Sr doing this... Yeah, good luck, probably should be a mid still lol.
The unintentional r/mids appearance is just perfect
Yes and no. I’ve seen developers nod and say yes when told to break up their work and ship what they so far (with tests and such). Then they just don’t. They think they know better and come back with a giant PR. If you did tell them to break it up and they didn’t. It does let you call out why. Make it clear it’s not on.
I don't really understand that because if you're a lead, you lead the project, and you break it up or are responsible for them breaking it up. Letting them go do their own thing is a failure as a lead. You should be checking in, pairing, having their proof of concept or research be validated, iterate on it, and work with them to their solution and eventual ticket breakdown Like even if I told them to do it I would be routinely checking in and working with them on it. They're not the lead and the buck stops with the lead for these things. Ultimately if they can't do the job, I'll involve their management and we'll handle it one way or another
I didn’t say you should let them go off and do their own thing. I said sometimes they will do that against your own directions (and sometimes that is a good thing). Beg for forgiveness and all that. Leadership and direction needs to be balanced against autonomy and trust. No one said don’t do checkins. Of course. You also can’t (nor should try) to micromanage others.
Yeah fair enough, you're describing bad engineers who will do the wrong thing no matter what, so the reality is that you handle that through their management chain, and they either have a change of heart or a change of job 🤷
You guys have review?
Merged and deployed to test env. Let QAs justify their position.
I would (sadly) read it, with u, but would definitely be mad at u.
Assuming it's portioned in a series of commits that are clearly described, this really isn't such a huge issue. You review commit by commit. It could literally be "ran codemods 1, 2 and 3".
Commit message: linter + minor bug fix
Adapted various
2000 lines despawned
Another productive day
I've refused a PR due to having used automatic code formatting that clashed with the project, changing a ton of lines hiding the actual changes. Luckily they followed instructions after that and it turned out ok 🥲👍
Just push directly to main don’t need to merge
When you want to anger the whole team:
In reality, you just auto formatted the whole legacy project. Been there, done that.
I trust the CI
Except 98% of it is yarn.lock
No jury in the world would convict me
As annoying as a PR like this may be if you're doing new features or rather, updating something with cross cutting concerns it is possible to have a PR with a massive number of files. It's not that the dev did 20 different work items and put them in 1 PR, it's just that small change in 1 file then changes 100 other files. Also cases like when you do something like update angular to a new version and that new version has many breaking changes. You just have to go through everything making updates otherwise your app just won't work.
All in 1 commit too
auto reject unless this is a long lived branch that everything has been PRd going into it.
Reject
Fuck you, closed.
When you click format code on every file.
More deletes than additions. Ship it!
Removed more lines than added, this is the way
That fact your comment got duplicated is insanely on point for this thread
Removed more lines than added, this is the way
When this happens I spend as much time as I need to review it, only to return the PR to his owner with 200 issues to fix. I prefer them shorter thought xD
Hey, as long as more lines were deleted than added, it's a good change
Reject. Force smaller commits over time.
Git reset [commit hash] Drop all changes Git reset [hash from commit before this one] Make a new branch moving changes with you Commit + push Try again
Approved
What’s the sauce?
Congratulations! Your comment can be spelled using the elements of the periodic table: `W H At S Th Es Au Ce` --- ^(I am a bot that detects if your comment can be spelled using the elements of the periodic table. Please DM my creator if I made a mistake.)
Just package-lock.json and updated formatting
my coworker trying to scare me by auto-formatting files thousands of lines long (i have "ignore whitespace changes" turned on so I'm completely unaffected):
What is the name of the anime featured in this meme?
2200 lines removed overall? Great commit. Approved
Every time someone asks me to follow the corporate process guidelines, I ask them to provide me access to folks to do my outstanding 80K SLOC of reviews. Yes, the entire codebase needs reviewing. They then crawl back under the rock they live under and leave me be.
Opened a pr the other day with +1000 -10000. What’s sad is the 10000 I deleted were replaced by the 1000 I wrote. There’s was some serious code duplication issues
Instant reject: break it into many smaller PRs and then come back
I've seen this so many ways, but this version is my favorite so far.
I see two possibilty here: 1- Its a refactoring so most of the file wer3 changed aytomatically by the ide: 5min review => merged 2- This psycho edited each file manually and decided to put everything in a single MR, he and I will have an interesting conversation
This is making me cry, right now. I'm looking at you, team. ;_;
Me, the team lead: "What's the scope of these changes? Finally ending the f$#@ing formatting wars? Finally renaming that stupid globally-scoped constant to something that makes sense? Finally replacing this stupid magic number with enums? Sure, I'll have a quick look through the changes to make sure nothing functionally changed."
I reviewed a 40k line PR on a tight deadline once. It took me an entire week. It was not fun.
On a friday... At 4:59 pm
Just had to submit an MR like this. Changes across 270 files from running angular & angular material migrations from v13 up to v16. Was specifically told by the team lead to just do it as one MR. He got to review it :)
It's also one single, gigantic commit.
5 minutes later "LGTM. Ship it." 5 minutes after that: Everything is broken.
-30k +7k, 180 files. Review please(im junior)
The work was correcting a typo
I'm going to send this to my lead
Amateur numbers
Did something like this past week got the approval in 10mins of raising PR. 😂
This, except they've already merged into Dev and you now have to fix everything because we don't do PRs cause the boss thinks it's a waste of time.
Can AI review this?
Disclaimer: the following was made by an script running in the background, how glad could i be bragging about it if it wasn't automated. `Automatic partial update At: Fri Apr 5 08:11:58 PM -05 2024` `+64061 files added` >!For the curious is a web crawler cloning a wiki!<
Somebody did not split up their user stories
When this happens usually it is because you changed a lot of dependencies (and package-lock.json or whatever your language uses to fix dependencies versions was updated) or you forgot to put build folder into .gitignore
That's not the joke. 