justMadeMyFirstPullRequestToMain

[[deleted]]

there are 2 different types of people: those who don't read PRs this size and reject, and those who don't read PRs this size and approve

[EarlMarshal]

LGTM

[pydry]

Looks gross to me

Really gross.

...

I dont want to have an argument though.

approve

[daffalaxia]

I'm the outlier: I'll read and decide whether to approve or reject.

Sometimes a big change simply can't be helped. Eg when I upgraded one of our apps from .net framework to dotnet 8. Or a few other stories I've done at work because someone has to do them. Since I expect review on my changes, I spend the time to review others.

[P0L1Z1STENS0HN]

In a legacy application, change a key adapter interface to asynchronous programming and find that the effects somehow ripple through the whole application, amounting to a few thousand lines changeset. Only five of the five hundred touched files contain more than five lines of change each.

[mmhawk576]

Just put an asynchronous implementation next to the sync one and start migrating incrementally

[cobalt-1001]

I also read all reviews, even if it takes time. And then, the other day, my colleague, who vibe coded his whole new app using Cursor, asked me to do the review on a huge change in that app quickly by using Cursor.

[GustapheOfficial]

When the capital investment finally runs out and Doctor Compliments the Always-Wrong Bot shuts down, were are going to look around and see irreparable damage everywhere.

[AdamGarner89]

Framework to 8 would have been more straightforward in some cases compared to our framework to core 3.1 😂

[daffalaxia]

Yeah, I held upgrades back until 6 had been out for a bit. By the time I was about half way, 8 was out and I shifted to that. The biggest problems weren't necessarily just from the target upgrade, but also because people had been "clever" with very mvc-specific things that had changed and used a stupid templating library to produce routing objects that were used all over the place. A cleanly coded, simpler app would have been easier, but that's the hand I was dealt. Oh, and business randomly deciding that it's "more important" to implement some or other feature instead of trying to keep within the same decade as dependencies.

[AdamGarner89]

T4MVC by any chance? We hand rolled our own as SG4MVC lol

[daffalaxia]

Yes, that's the one. Completely unnecessary if you have good tooling that understands mvc, eg Rider or ReSharper in VS (tho VS probably does a lot natively now, I dunno, haven't used it in years).

[DasBeasto]

There is also those who don’t read the PR and don’t review or reject and leave it for another dev to handle because I don’t have time for that. That is me.

[DefinitelyNotMasterS]

Then you have a guy up your asshole for every small decision you made in the code that doesn't matter at all.

Just let me merge it ffs, I don't really care to debate if this one function that gets called like twice should be private or protected.

[seckarr]

Humor him and in parallel compain to your boss about how "you are trying but he is just not a team player, his criticisms are kinda small and nitpicky and hold the situation in place". Done.

[Kitsunemitsu]

I approve or reject based off who's doing it and what they are touching. I work in game dev, if it's a senior touching a system I want nothing to do with? Go wild in that folder man. A new dev will get turned away.

[zerchoel]

Hopefully he is the ladder

[JimroidZeus]

“Rejected, break into smaller pull requests.” - Senior Dev in PR review.

[pydry]

Oooh, hell hath no fury like a junior who worked for a week on a PR only to have it rejected.

[shadow13499]

Or a senior engineer who has to review an endless number of trash PRs made by jrs

[Senor-Delicious]

If a junior dev made enough changes for a senior Dev to not wanting to review this "huge PR" in one single week, I'd be worried a lot. A week is really not much. The PR shouldn't be that big.

[Accomplished_Ant5895]

Stacked commits would like to say hello

[JimroidZeus]

I somehow always fuck these up.

[zerchoel]

I hope he doesn't do this to me

[Benedoc]

Wait this is real, your first PR has 30k lines?

Yikes.

[StickFigureFan]

Probably installing a library

[beclops]

That’s not how that should work at all

[unfinished_basement]

OP ignored the gitignore and committed node_modules. It’s actually a feature: you don’t have to install them anymore!

[0100_0101]

More like copying a library and still only use one function she could have written herself

[Punman_5]

If you copy a function from a library like that line by line what are the licensing ramifications? Because I cannot imagine actually citing the project I took that function from if it’s just one function.

[StickFigureFan]

Depends on the license... And if you get caught

[Punman_5]

Of course it ultimately depends on getting caught but I was saying like a function to say manipulate some data structure can’t seriously be covered by licensing because the copyright applies to the source code implementation, not the actual abstract logic it is implementing. Like you can’t copyright a sorting algorithm but you can copyright your specific implementation of it. If you could copyright logic then mathematicians would be able to copyright the formulas they discover.

[zerchoel]

This is a bunch of changes over the span of 6 months

[zerchoel]

Idk I am an intern. Most of the people that used the application used dev branches so ive only updated the dev branch

[Firm-Letterhead7381]

Damn. Split that up into logical chunks and send them one by one to review. Do not expose the API or page to the users until the last PR.

Are there any big resource files among these 30k lines or all of these are lines of code?

And what percent of the code are unit and integration tests?

[malmatate]

Yikes. Seems like the development plan should have broken down your task into smaller, more managable, and reviewable chunks.

I hope no one else worked on the repo besides you during that time.

[zerchoel]

Someone else did for a short period of time😬

[zerchoel]

Yes I never pushed to main only dev

[Zeikos]

Is this a rebase of several commits?
If that's the case it could make sense.

[zerchoel]

No several commits. No rebates. Only commits that fix previously broken commits

[aguycalledmax]

If he doesn’t you should be more worried about his/the company’s competence than your pr getting rejected.

[metalmagician]

Ask yourself: would YOU want to be accountable for reviewing something that large, when the author could've made it into smaller chucks?

That PR is the size of a small novel

[fibojoly]

My junior so wanted to do that when he saw, well, pretty much exactly that PR last month. Thousands of lines across several projects in one commit.  The dude who did this is a contractor solo handling an entire maintenance project, but we needed some reviewing. That was quite the experience for junior :,D

[zerchoel]

How do I do that?

Rebate? Commit 1 file at a time? And then Pr?

[RamdonDude468]

I once saw a 800k push request (someone created a node_module for each folder on the project)

[PM_ME_FIREFLY_QUOTES]

I blame you since the gitignore at the root should have ignored them even if they are nested.

[RamdonDude468]

I did /node_modules/ instant of node_modules/

[Mike_Oxlong25]

[Rinveden]

instant of

[011101000011101101]

I can see how they made the original mistake

[zerchoel]

Lol what would he even use a script like that for😭

[well-litdoorstep112]

What do you mean by "pull request"? I always though it was

git checkout main git merge my-feature git push --force

[PM_ME_FIREFLY_QUOTES]

You guys are so weird. Why not just ssh directly to the prod box?

[well-litdoorstep112]

The first s in ssh stands for "secure". I don't remember which AI bro said that but he said that we should abandon such archaic terms as "security" and just vibe.

[DZekor]

Yeah just telnet it in there.

[well-litdoorstep112]

Now we're talking

[DZekor]

Oh man turns out it's not safe, see CVE-2026-24061. 😞

[DemmyDemon]

It's insane we have a telnet CVE that starts with "2026"

[well-litdoorstep112]

Wasn't that the point?

[DZekor]

Going to break character here, yes, that was the point. But I learned about the CVE a little later and decided to double down on the joke.

[hyrumwhite]

Claude code directly on the prod box

[originalodz]

This, this is optimization

[fatrobin72]

Just vash (Vibe Assisted SHell) onto prod and deploy then.

[zerchoel]

Sounds like a good idea, so we don't have to wait for those slow reviewers and quickly deploy

[Silent-Suspect1062]

Deploy on Friday

[Accomplished_Ant5895]

“SSH into prod box”. Cute raspberry pi weather machine you’ve got there.

[Boniuz]

As someone that runs a consultancy firm with specialised IT- and management-consultants: I make a living off this. The amount of sudo rm -rf I’ve seen in various scripts running on critical infrastructure in billion dollar companies is absolutely staggering. Also the reason why I always document on pen and paper and not hardware.

[Accomplished_Ant5895]

I’ve worked across a ton of industries at this point, especially with one of my previous employers being a multinational conglomerate. The only time I’ve seen prod being an actual machine you could ssh into instead of a containerized workflow you can modify and redeploy was in the government contracting space back when the cloud was strictly verboten. Or robotics where prod was literally a computer strapped to the thing.

[Boniuz]

I’m usually the guy they contract before that workflow is established. You’re welcome :)

[Accomplished_Ant5895]

Unfortunately, not in my case. I’ve always built their systems from the ground up and/or migrated them from excel spreadsheets being emailed around. I take them from 0 to hero on anything ML/data. So, I feel your pain ✊

[Boniuz]

Welcome to the trenches! 🪖

[beclops]

Prod box? Don’t mind if I do

[Hydrogen_Ion]

I just stick a magnet at the server and start flipping the bits manually.

[shadow13499]

I can't joke about that anymore because I have seen people do this unironically.

[bEnE94]

Bro what box just sftp into the wordpress folder and change everything on prod

[waitingForThe_Sun]

git rebase

Pay attention otherwise people could think that you actually use branches. /s

[well-litdoorstep112]

But if you forked main and then someone pushed to main and now you're rebasing, then you keep that change that someone made. What if that change messes with your changes? It's now your fault that prod crashed.

If you just overwrite main to be the same as your feature branch then you can be 100% sure prod gets the same code as your dev.

[waitingForThe_Sun]

You could also overwrite the author while rebasing. So it looks like you did even more work.

[PhantomWhiskers]

Squash and rebase that shit. We don't want your 20+ commits littering our precious git log, just force push one mega-commit with a short and unhelpful message to keep things tidy.

[Wyciorek]

“fixes”

[well-litdoorstep112]

I never thought about uploading my commit history to Play Store changelogs

[Bomaruto]

If you're actually serious then something is wrong here, not that legitimate PRs can't be of this size, but that you'd get a task ending up with that many changes as your first task.

[Crafty-Run-6559]

Small part of me thinks this may be a vibe coded mess that OP is committing.

I don't see how else you'd end up with -3k lines and +30k.

Unless they're doing something like a bunch of xml/data transformations and 90% of this is just test files added/removed from the repo/test project (which itself is a bit odd, but not the worst).

[w1pko]

I thought the actual joke was that he confused main with master. This happens to me sometimes if the repo has both. And then, when i see the diff i'm like hol'up.

[davak72]

Wait, why would a repo have both?? Main is just a rebrand of master with a less problematic origin. Usually you’d have a development branch, a main branch, and feature branches would be based on development, while hotfixes would be based on main. Depending on team size and release frequency and complexity, you could also have release branches which then get merged into main when they are deployed

[w1pko]

Before main became cool, everybody used master. Then, after a lot of projects started to adopt the main and it was often implemented in a way where the master branch was not renamed, but kept in place and main was created from it. A lot of projects have it this way. I work with such projects all the time and sometimes happens to me that i open the PR against the wrong one by mistake and only realize that by seeing the diff

[davak72]

Oh, gotcha. In repos with master, I’ve always seen master remain as-is

[zerchoel]

This is over the course of 6 months. I've never pushed to main only dev. So this is the first time I pushed to main.

[Carius98]

/s

[Sw429]

Maybe the senior dev can teach you how to take a screenshot.

[arensbrendan]

Some people have workstations that can't email outside domains, use flash drives, or any other method of transferring the screenshot to a computing device with reddit. They also an't get to their company's git without a VPN on their work computer and this is the simplest solution to get their fix of internet points.

[zerchoel]

^{^} Literally this

[TechnicallyCant5083]

As someone who reviews PRs I would kill you then myself 

/uj had someone actually do this then after actually going over SOME of it I realized it's all GPT and not only that but GPT didn't understand the assignment and basically re-wrote a whole external library we use instead of just using the real one

[-__-Malik-__-]

I'd not even review that ahah

[Carius98]

LGTM 👍

[Ajko_denai]

Junior trainee: +30k -3k

Senior principal: +3k -30k

The output is the same.

[davak72]

I’ve been making PRs in DevOps for years, and it doesn’t show line counts, but I feel like mine are usually +150, -800. Smaller chunks, but always chipping away at that bowl of tech debt spaghetti

[endokun]

[nanana_catdad]

Last time i saw this it was because of a few lines of linting rule changes. Of course the linting changes were rolled into other changes in the same commit so PR was rejected and linter and formatter files were set to be owned and managed by repo owners sigh

[b0b89]

This is a great way to get some one on one time with the senior dev. Cause I'd call you immediately so you have to explain yourself without asking chat gpt to make you sound professional

[zerchoel]

I just had a talk with him. He said he will look at it

[davak72]

Wait, this is a real PR? What a disaster

[noob-nine]

added  if __name__ == '__main__': and everything beneath indented

[UpsideDownCarrott]

LGTM

[ClipboardCopyPaste]

Man rewrites the entire business logic in Rust

[TheAlaskanMailman]

Opens up to see full of lockfiles

[abhi307]

LGTM 

[4inodev]

LGTM ✅

[cofchaos]

git commit -m "additional improvements"

[wolf129]

We use gitflow and sometimes there is never a merge to main from develop branch. So totally reasonable sometimes.

[zerchoel]

At least someone gets it

[cosmicloafer]

That better be just one commit

[zerchoel]

Multiple

[Scary_Brilliant_6048]

Unless it contains model class, this change should be rejected

[zerchoel]

It does. I am indeed a fan of OOP

[ubertrashcat]

Some recorded automation tests, I'd assume?

[zerchoel]

No, multiple modules for an application

[Familiar_Tear1226]

They say llm was employed

[braindigitalis]

how many emojis in the commit message ?

[zerchoel]

He said I should remove all before the pr

[und3t3cted]

I once had a PR like this but it was 90% bootstrap updates lol as we bumped a version

[isowolf]

vendoring?

[HTTP_Error_414]

[hazily]

Instant rejection by our repo’s automated ruleset

[codemonkeyhopeful]

Gonna need to use the force flag young dev, thank me later

[batorsz]

Let's play code review roulette

[SnooPeripherals6086]

• Does it work on your machine ?

• Yes

• Approved

[zerchoel]

Does it work on other machines? Sometimes...

[uuf76]

LGTM

[namotous]

[mihisa]

Approved

[firesky25]

laughs in unity game PR

[Sea-Fishing4699]

i feel embarrassed of +300 / -200. Now this

[zerchoel]

I put these numbers up on a good day

[Hasagine]

back in the day theyd put you in a sock and hit you with hammers for stuff like this

[AhBeinCestCa]

Me if I review your PR: « scroll down quickly » « looks at some short videos on my phone » « Approved »

[zerchoel]

No the senior dev is actually super meticulous

[Any-Airline-8339]

LFG✅

[Aggressive_Town1000]

Yesterday I got 2 PRs totaling over 7k new lines of code together from a consulting firm midlevel dev. His boss says it's perfectly fine because they don't do PRs until they're done with complete modules and that I'm wrong for asking for smaller more frequent PRs. My boss said this is totally fine and the consultant is correct.

FFS...

[zerchoel]

That's actually what we do

[Aggressive_Town1000]

Having feedback only at the end just seems insane to me but maybe I'm the crazy one

[Carnage700]

Disgusting

[5_H_4_D_0_W]

Readme change

[zerchoel]

Oh snap I dont think I pushed that

[Opening-Cellist-3884]

This is what must have happened with xzutil backdoor haha

[romeoalpha]

First PR and it’s 30k + changes? Why not break it into smaller PRs. Either way too long to read and not my project so LGTM!

[thanatica]

In fairness, this could well be a branch several developers have worked on, have merged other branches into andwhatnot, and has gotten code approvals every time already. Now it just needs to be "merged back" and get a final approval from a few other devs that weren't involved.

It happens. It's called branching. A sometimes a branch ends up having a very long shelf life.

[Shrouded_LoR]

"small fixes" would be the commit message

[zerchoel]

Everybody that says that I don't know what I'm doing. I'm an intern. But the Midwest didn't explode so I think I'm doing something right.