What's the largest MR / PR you've had to review?
Posted by NameGenerator333@reddit | ExperiencedDevs | View on Reddit | 63 comments
Title says it all. I'm dealing with some nonsense at work. I'd like to hear some horror stories, so that I don't feel so bad about my situation.
Odd-Investigator-870@reddit
2,000+ line PRs - from PhD data scientist types (the worst) - who were left to themselves for one-three months - who hack something together that improves a model's accuracy from 90% to 90.5% - and when confronted with quality standards, get angry and turn full Karen - and seemingly the only person who has authority to stop them is a director, because they see how much more costly that model would be to train and serve.
TLDR: PhDs who never learned to work as a team, refuse to learn to code competently, and have an entitled attitude. The same ones who think jobs with bold "Machine Learning Engineer, do not apply if you're a data scientist" doesn't precisely refer to their ilk.
Superiorem@reddit
I’ve worked my entire career adjacent to PhD/data scientists, and I’m afraid it’s having a negative impact on my growth as an engineer.
They are generally really smart people, but they have spent more time thinking about statistics and other mathematics than producing software systems, so everything they do write is just R&D mediocre code.
Jupyter and Pandas are not the solution to every problem. :(
GuyWithLag@reddit
I stopped reading there due to PTSD.
Fun fact: on average, PhD holders aren't really smarter than the average Masters' holder - they just were willing to put up with the grind, had funds, or connections.
neuralscattered@reddit
I always encourage PRs to be as small as possible, but the largest one I've had to deal with was probably 3k+ lines. Sometimes that's unavoidable though, due to a lot of components depending on each other, so they all need to be modified in 1 go. IMO it's good etiquette in these cases to rebase your commits in a way that it's easy to follow the commit history to understand the logical flow of changes, if you can swing it.
binarycow@reddit
I recently had a PR that modified over 3,000 files. I don't know how many lines, because GitLab gave up.
Luckily, almost all of that was moving thousands of files one level deeper in the directory structure. And I did all of that in one commit.
Handle-Flaky@reddit
uhh there shouldn’t be any loc change if you git mv
binarycow@reddit
I didn't. I didn't switch to the terminal to type some other command.
I dragged and dropped in my IDE.
GuyWithLag@reddit
Amateur. I once created a PR of ~77k lines.
Granted, it was a total replacement of a microservice, was developed over several months, ~30% of the total LOC were test data, w. 40/60 code/tests, and ~200 PRs on that branch.
Oh and yes, we were shadow testing it in prod beforehand.
SoInsightful@reddit
I have a CRA-to-Vite PR up right now that technically has +22263 -34329 changes. Anyone care to review?
tetryds@reddit
Rewrite to the entire system. I painstakinly reviewed every single line of code and added 50+ comments. Was fun, I like to review code.
GuyWithLag@reddit
... you mean, 50k comments, right?
tetryds@reddit
I was a nice person so most of them were "fix for every time this same thing appears" so that cut the comment count by at least half
ReservoirBaws@reddit
Bad offshore contractor who spent a week on his task, essentially forked the entire repo, so his PR was a few hundred files. Management didn’t want to admit that the push for offshore wasn’t working, so I did maliciously comply and review it. (The code didn’t compile anyway).
GuyWithLag@reddit
depending on the tool, you should be able to add their repo as just another upstream.
latchkeylessons@reddit
Holy shit, I'm doing the exact same thing right now. There must be many of us.
tallgeeseR@reddit
Once I joined a new team in mid sized tech firm, opened few commits in history to investigate a production issue. All commits I tried to open was from single-commit PR. It drove me crazy because github wasn't able to open any of the commits, with error commit's changes too huge. Eventually found a workaround, open each file change in the commit one by one.
It wasn't due to whitespace formatting. Instead, it was due to a direction from the team's ex-techlead - once a PR is reviewed, before it get merged, all commits in the PR have to be squashed into single project commit to make git commits history looks cleaner. A PR may contain changes from all team members of the project. Thanks to that direction... we 1. Struggled to examine past code changes whenever needed. 2. Unable to trace when exactly a line of code was introduced. 3. Unable to trace who introduced the weird code so can't seek clarification.
I was really shocked when my new teammates told me that the ex-techlead who already left was a staff engineer from faang.
Technical_Gap7316@reddit
I find that a religious devotion to small PRs ultimately creates a bigger mess than the occasional whopper.
Small PRs get rubberstamped half the time anyway because it's a pain to validate each one yourself.
As long as there are proper testing and deployment practices, I don't really care how big the PR is.
pigtrickster@reddit
I joined a team and looked at a particular piece of code with absolutely zero tests. I asked around about it and people just shrugged. One of the team said that it's a right of passage to read it and work in it - everybody wants to rewrite it but it's too core, complex, obtuse to really re-write. Needless to say I tried anyway. A few times.
Finally, I grabbed another engineer and we paired through it. A single class turned into something like 70 and they all had testing. It took two days.
This solution had a built in code reviewer for what would have been impossible to review properly and get approval for any other way.
thekwoka@reddit
I find the opposite. It's kind of the issue of bike shedding.
Small PRs get more attention because the reviewer is capable of understanding it, and large ones get less, because it's harder to understand it.
reallybrutallyhonest@reddit
At my org I find the opposite - 80 liner will have 10 comments to address, a 500 liner will have about 5. My colleagues seem to exhaustively review every line on a smaller one, but once it's a bit bigger it's more of a skim.
Frankly, I'm not sure which secnario would lead to the a worse outcome.
Technical_Gap7316@reddit
Yeah, I've definitely seen this, too. IMO, it's usually a sign of a dysfunctional team.
Small PRs are either rubber stamped or agonized over. I think that's what happens when there's just not enough meat on the bone.
tcpukl@reddit
10k files probably.
Back in the day game studios around the world had different repository around the world. But each studio has their own local version that needed merging into our local one every day by hand. Then had to copy up our edits for the past day.
This was one person's job at the beginning of every day.
unflores@reddit
500 Loc With new lib 30k Loc.
flerchin@reddit
It sounds like you're calling him stupid, and his work garbage. Can you see why he might not be receptive to feedback?
Instead you might approach him like you're on the same team, and have a problem you want to solve.
My Hank regularly produces 50k SLOC MRs, and we make a ton of money together.
i_dont_wanna_sign_in@reddit
I'm guessing this has happened to more than a few people. I used to work for a telecom system. After a zillion mergers there were about a zillion CRMs tracking customer info, so you'd have to search through all of them to find customer data when they called in. Several efforts were made to bridge all the systems over the decades but they ultimately fell flat because VPs, SVPs, CxO, and Presidents would much rather bicker and argue about how to do something than make a decision. Ever.
So this dude, we'll call him Hank, who has almost no development experience whatsoever decides he's going to write a CRM system to track customer circuits and info for OSS so they don't have to look though a zillion system. He writes the entire thing with Flask/SQLAlchemy in Python 2.7. This is a few years back when 2.7 was still considered supported but well after 3.x was rolled out and 2.7 had a very real death date.
So Hank cowboys this entire application. Thousands of lines of code to track source systems and make everything searchable. He gets access to most of the systems by piggybacking off of service accounts and such so that he can glean info from these systems that most other attempts are blocked at. I'm all for black ops projects getting stuff done. Dude spent a LOT of time at my desk as I was the most senior developer in the department and I honestly had no idea what he was doing, just helped him clear technical hurdles with code he didn't understand.
Okay, FF a couple years and in yet another reorg I tell my boss we should absorb his app to avoid getting folded under this other VP who was a known a-hole and did not like us. There wasn't a lot of time to do any research, I just knew that Hank was a nice guy, seemed to accept my critiques in stride, laughed easy, was appreciative, etc. We didn't have time to look at code or performance, just an hour meeting and this was thrown at us.... Our job would be to get everything up to snuff and put his application in the official environment.
Big. Mistake.
Hank has been a cowboy for a LONG time. Hank's application barely runs. It has to be "turned off and on again" to "fix" problems left and right. Hank is stuck in an endless loop of support because the thing barely holds together. Hank has written a LOT of code and we can't onboard it because he has ZERO code coverage. Zero. Not one test. Nothing. Just a memorized workflow that he continues to modify in his head as features and fixes are implemented.
Hank does not like working with others. Hank does not like documentation. Hank does not like feedback. Hank develops in Notepad++.
Hank's full stack application goes into code review within a week of the reorg. The first thing I do is lint his code and I swear the linter got half way through the second module, stopped working, stared into the distance, pulled a gun from the aether, stuck it in its mouth and pulled the trigger.
Hank's code, for the most part, broke PyLint and caused it to generate negative scores (which I've seen on occasion). Hank's code went into the static security tools and came out with more critical security problems than lines of code. Hank was a big fan of letting user input go straight to the CLI using "eval".
I took fixing his code in phases. Overall it took a couple months before I was willing to touch it
At this point we'll start to review code. The linter stopped committing suicide, but all the security problems were there. We spent a couple more months cleaning up his custom queries so that they were readable and finally, after nearly a year, we were in a place where we could commit his code to a development instance and start working on the hundreds of critical vulnerabilities.
yeastyboi@reddit
I've met people like this before. Its shocking how arrogant a lot of these developers are. I worked with a similar low skilled dev in a 250k codebase. No object oriented programming, barely any functions (he would copy and paste then modify), 1 letter names as well, horrible performance (9 minute page load on some things). Eventually he was so rude to me I had to hit him with the "you've been a developer for 10 years yet I've met college sophomores better than you. You don't get to be arrogant.". Now he's scared of me, but luckily he takes feedback and tries to improve.
i_dont_wanna_sign_in@reddit
We took over this team and there was a contractor in it who was, well, he was rocking a combover. Python codebase and he would not do subtly send me articles about how OO programming was wrong. Incidentally he also used Notepad++ for development. Never committed anything useful in the month or two before my boss cut him loose.
Few-Conversation7144@reddit
It’s ironic how the junior devs always blame problems on OOP and go fully functional. Had a similar thing on a few teams where OOP was banned and it’s always an early warning sign
NameGenerator333@reddit (OP)
Thank you! This does make me feel a little better.
Sauermachtlustig84@reddit
Eval can insanely powerful but it's also an extreme foot gun. Best use case I've ever seen: python for work from database a to database B. The rules on how to convert some fields was super complicated and changed. A user knew enough python to be useful. So we added a column for him where he could store his transformation formula and evaled that. Super useful for all involved.
stillbornstillhere@reddit
That's hilarious, I still can't get over this:
Reminds me of a post on here where someone joined a new team that insisted on writing GOTOs in C#.
Self-modifying assembly and manual control flow... did these guys learn to code before Algol 68 dropped or something? What’s next, patching the stack by hand with a soldering iron??
i_dont_wanna_sign_in@reddit
There was a HUGE fight over the use of eval. He swore it was sanitized and I brought his dev server down without really looking at the code by guessing where I could insert double ampersands and deleting the contents of the current directory. He also made a habit of parking data in the temp folder which I erased for him.
I flat out told him all usages of "eval" must be removed and if he can't figure out how to initiate his dynamic queries than I had other developers who could (probably, nobody wanted to touch Hank's stuff). He said it was impossible, no way to do it. I showed him half a dozen existing python projects that do exactly what he needed, pick one.
I did let him try to justify its use, too. I gave him the security notices from our 3rd party auditors and told him he'd have to defend it from every angle before the next audit, but his usages were indefensible.
kutjelul@reddit
I have nothing of value to add, but this was an interesting story
AIR-2-Genie4Ukraine@reddit
around 2 megabytes in size, 2 weeks. ]
90s ahoy
Stubbby@reddit
Does a complete scrap of 40k LoC and rewriting by setting up new architecture count? It was technically a PR.
dring157@reddit
New engineer said that he thought the interface and implementation for a service I wrote needed improvement. The service was around 2K lines total. The interface had around 5 requests and 5 responses.
He wrote a new interface with 50 requests and 50 responses, which was around a 5K line PR. I refused to review it, but he got another guy on the team to approve it. He rewrote the single client that used this service and saved around 10 lines.
He then redid the way the service stored data. Requests needed a bulk of information on 1 ID at a time, so the data was store with 1 row per an ID that contained all the information. There were around 50 IDs. He decided to split the information up into around 20 tables, so he’d be doing 20 DB reads for each request instead of one, but he insisted that this was cleaner.
After around 6 months the service was more than 20K lines and he decided that it was good now. The service had not functionally changed at all and requests were handled significantly slower, though this wasn’t really an issue, because it was a low usage service and it had started out quick. I didn’t approve of anything he did, but my manager somewhat reluctantly let it happen.
He became the service owner, but would regularly ask me what the service actually did, because he had never bothered to try to understand it. I would tell him that I could describe what it used to do, but I couldn’t possibly know what it does now, because I wasn’t going to read his code.
cloakrune@reddit
Few thousand lines. Massive infra update. Was a really really good change. We did the initial breakdown to do it over time, but it made more sense to do it all at once. Helps that its embedded and I can manage the complexity within my project pretty well.
me_again@reddit
I think the biggest single PR in our code base was mine. I ran the entire thing through a code auto-formatter and made it so all future commits would be auto-formatted. So there were a LOT of lines of code, but folks were willing to trust that it was all formatting and no actual changes, so it got through OK.
The grumbling didn't really start till everyone had to start fixing the merge conflicts in their own branches.
I'm not sorry. I'd do it again. 😈
Devboe@reddit
Have you ever had to update a third party dependency that isn’t backwards compatible? Yeah, there’s no reviewing that. You just check out the branch and if it compiles, give it the thumbs up LGTM.
kellogs4@reddit
I don’t review prs more than 15/20 file changes
Ciff_@reddit
If you have pressed deliveries with a short financial runway with an highly coupled codebase, that is not a luxury you can afford.
kellogs4@reddit
I don’t agree, you can still split your changes into sensible PRs, no need to do everything at once
Ciff_@reddit
binarycow@reddit
So what do you do when all of the changes depend on each other?
Wooden-Contract-2760@reddit
You don't, but someone will have to, right? What about updating a widely used package or improving an extension method that results change in calls?
Southern-Reveal5111@reddit
It was around 8000 lines of code, and the review was done at the last moment of release.
The management made a decision that the feature shall go for testing, the developer asked us to review. No one was looking at the PR. Then they asked me to do the review.
It had more than 200 review comments and some of the changes made our coding guideline a joke. The developer refused to address the feedback and escalated the issue to the director, claiming that the team was being overly resistant. The director then asked me to negotiate. In the end, I removed the review comments, and the PR was merged as-is.
Some of the review comments were
- A lot of the code can be moved to the GPU to speed up computation.
- New is more preferred in C++. Smart pointers are better than manually deallocating memory.
- Too many nullptr checks, you don't need to check for nullptr if the member is a const and the constructor guarantees a valid address.
- We don't need move semantics for stack-allocated primitive values.
- The functions do not need to be 200 lines of code, and it does too many things.
In the one-to-one meeting, the manager told I should always try to align my goals with product management, and my review comments are hurtful to others. That PR reduced the frame rate in the rendering, and they blamed a guy who left the company 6 months ago.
annoying_cyclist@reddit
Low five figures LOC, hundreds of files, no commit discipline, no particular rhyme or reason to what was in the PR, no real thought given to risk management, a test plan, etc. Essentially a newer engineer's rage rewrite of a complex, business critical component that they didn't understand, in a bigger system they also didn't understand.
Took me a couple hours a day for couple weeks, but I did eventually give it a quality review. We scoped it down to something reasonable (still huge, but tractably huge), I caught a number of expensive defects before they hit prod, I helped them come up with a test plan, and it went to prod without much fuss. I resented this engineer at the time, but in retrospect I'm proud that we actually got that smoothly to prod, and the process gave me some new tools for my PR reviewer toolbox.
In retrospect, I should have just put my foot down. That change was unacceptable for risk and other reasons, and I just enabled the behavior by reviewing it. Kind of a case of "pick your battles" at the time, though. My manager – distant from the details –thought that I was saying no too often to this engineer, and their change was solid conceptually, their execution just kinda sucked. It felt like there was an unacceptable cost to saying no, and I'd have been failing as a TL if I'd let someone else rubber stamp it and shipped the original defects to prod, so... 🤷
icenoid@reddit
Largest I’ve had to look at was something on the order of 10k lines of code. QA architect decided to move the full set of front end tests from Python to Ruby. He also didn’t like ruby’s syntax, so he has all these weird aliases in the code to make it read like Python. It took like a week to make comments that he promptly ignored and merged anyway
PmanAce@reddit
Had a cleanup PR last week for removing feature flag paths that we had just released with 150 file changes. That's the most I've seen in one PR in our department and warned people in advance.
Usernamecheckout101@reddit
192 files and each of them contains 200 lines.. that is an automatic approval
pancakemonster02@reddit
I recently “reviewed” a PR that was 11,000 lines. Notably it was nicely broken into two commits: one was the codemod that changed 2 lines in 5000+ files I did not review and the actual change which was 20 lines.
Decent_Perception676@reddit
I just authored a PR with 150k additions/changes and 200k removals. Extremely rare scenario and I apologized to the team profusely, but it was necessary. Inherited a legacy code base with 10years of tech debt, stuck in an old version of React. Updating it turned to also dumping all the legacy/deprecated code, implementing a proper Monroe setup, and rebuilding all the build tooling and linting. Took me 2 weeks, but would have taken months and months as small PRs going through reviews.
GitHub froze on the file change page. 😅. Review was replaced with extensive testing.
Efficient_Sector_870@reddit
Insane, good job.
Efficient_Sector_870@reddit
Somewhere about 30k+ 30k- worst I saw
badfoodman@reddit
Tens of thousands of files moved, I don't remember the exact number. It happened twice, both kind of necessary to get us to a reasonable directory structure and onto modern build tools, but insane to work around because we had to backport across this boundary. To top it off, there was a bug in
git's machinery that prevented us from being able to cherry pick, so all changes had to be reimplemented by hand. It led to a blog post series where my team got a shoutout of sorts:At the end of the day, it was cool to be part of the fix to a crazy important piece of open source software, but that year++ where we had to deal with the situation was pretty brutal.
AmishMountaineer@reddit
A few years ago, my manager asked me to step in and help review MRs for a small 2-person project where the lead didn’t have time to review them for some reason, and the other team member was a junior engineer. The reviews were being done too slowly and they needed someone to step in to help get a few of them unstuck.
Each review was gigantic and since I wasn’t familiar with the project they were working, I also had to learn what they were trying to do in the first place to understand if the changes made sense for our codebase. I would start looking at an MR and get a feeling of deja vu that I had already seen the changes before.
Turns out, I really had seen the changes before. I come to find out that the junior basically was building each MR off of the previous MR, and he was also kicking off the MRs out of order and merging changes from one to another unrelated MR. The lead was basically not involved in this process so I spent some time explaining git branching and MR practices to the junior, and learned from him which order I should be reviewing the MRs in. The whole thing was a huge mess but eventually we got it squared away, it just took a lot longer than I assumed it would based on the initial request.
latchkeylessons@reddit
Working on one right now that's about 7100 lines. It sucks. I don't mind the tedium, it's more about the problems I know it will create down the road with other work in the codebase. But what do you do? Bad management be bad managing.
latkde@reddit
Currently in the midst of reviewing a PR that adds over 5k lines of code for a new feature. Some of it is buggy. Some of it is AI generated. Architecture is an afterthought. Tests don't run in CI because the CI system doesn't have enough disk space for some fixtures. We're 80 review comments deep, and will hit 200+ before this is over.
My strategy is to focus on slices of functionality that are somewhat cohesive, and iterate on reviews of that slice until its design is settled. Once it's clear how that part interacts with the rest of the feature, that part can be extracted into its own PR, and the review cycle can continue with the next slice.
In some areas of the code, I've found opportunities to draw clearer component boundaries. Having all of the new code together also made it possible to see that some functions had become unused during the review process. And I'm glad for this opportunity for mentoring/upskilling my team.
So not everything is bad. But dealing with this complexity is really exhausting.
Sauermachtlustig84@reddit
Consulting gig. Customer lost all his developers and his customers where demanding improvements.
Application was managing some iot devices and basic hotel stuff like " book room a for person x from date to date2".
Theoretically written in dotnet, the whole thing was a clusterfuck. Data access via linq2sql, which was deprecated before the project started. For the frontend they used wpf but they basically used it imperatively and ignored mvvm completely. Code was written like it was c. Especially fun was that they reimplemented crypto algorithms and a huge homegrown Bluetooth library which was interwoven with wpf code. Ah and they heard of Microservices - they had multiple services with a homegrown messagebus between them, which was based of raw sockets and was unable to handle more than one message/second.
Three huge commits: 1.just format everything. Touched every fucking File but was harmless. 2. Nuke the whole Bluetooth+wpf abomination from orbit and replace it with a sane approach. -1000k files, added a hundred or so. Ah and dropped the entire repo for their iot Bluetooth dongle and simply used the built-in BT of every laptop. Minor issue; they forgot to give their devices unique bt identifiers and built-in BT was so much better that it found hundreds of identical devices in the factory, making selecting the one in front of you difficult...
Few-Conversation7144@reddit
Hundreds of files were the largest. Average is probably around 20 files
I don’t think large PRs are inherently wrong but it should raise questions on how the codebase is setup if it’s too common
sbox_86@reddit
I've been in some six figure PRs. Usually this means we are pulling in changes from some upstream source, so we don't necessarily need to review everything, but we do need to look at files that were edited both upstream and by our team. Additionally, before merging we asked every team to download the check build from CI, run it, and ensure their components don't break. No matter how much testing you do, this is always a little scary to merge.
At my last shop I had a contract team that liked to do a big delivery from their dev branch at the end of a milestone, which meant routinely +10k to +20k PRs. I kept asking for smaller PRs but I had too much on my plate to ever enforce that. I settled on having them document a formal test plan and documenting their test runs before I approved PRs.
Inconsequentialis@reddit
A while ago we inherited a project and ran our formatter on the entire thing. I scrolled over the first 1k files GitLab shows you to check for any obvious fuck ups.
But the largest I've reviewed in depth... I've had a couple that were around 10k LOC in changes? Those have generally taken over a day to review carefully.
The most recent large one I remember was a complicated job from an old system reimplemented in a new system. The whole thing was so interconnected we didn't find a good way to hack it into parts, so it got implemented in one go. Production code was several thousand LOC, as were the automated tests written to ensure it still worked as before. Basically one months work of for one dev in one review. But was fun, I enjoyed that review.
If anyone knows of a good way to break stuff like that up into smaller parts let me know. We'd have loved to do it but couldn't figure out how.
PicklesAndCoorslight@reddit
Somebody changed the header in every single file.. and it had to be reviewed.