looksGoodToMerge

[u/soap94]

[u/SuitableDragonfly]

Just reject it with "please come back after the code freeze with this split into 20 separate PRs, thanks".

[u/NekoPricessGirl0 → u/SuitableDragonfly]

Or just type "LGTM", hit approve, and immediately shut your laptop for a two week PTO. It's the weekend on call guy's problem now

[u/MissinqLink → u/NekoPricessGirl0]

As the on call guy,
https://giphy.com/gifs/13ROLRgvNf5WQo

[u/d_maes → u/MissinqLink]

17:30 - pushes code, closes laptop, not my problem anymore

18:00 - New notification from spike.sh: your oncall has started

Fuck

[u/Escanorr_ → u/[deleted]]

Yes, yes it is. I didn't even have to pump up PR's this big for new features in graphical processing pipelines of game engine, and those were massive. Everything is splittable, if your test suite forces you to make large bloated PR's it's bad test suite. In my career I've worked in game dev, software houses, webdev, embedded and iot and everywhere I've always been able to make my PR's compact and readable.

[u/Entropius → u/[deleted]]

Does 20 seperate PR actually make releasing such a large new feature any easier / better / faster?

Yes.   It dramatically improves the quality of the code review.   Reviewers eyes often glaze over after about 500 lines.   After that, mistakes that ought to be caught in review likely won’t be and you’ll end up more likely to get a flippant “LGTM”.

So my team was encouraged to keep the PRs small and add more PRs if necessary.   This wasn’t always done, but we’d try to whenever possible.

I once had a change that I ended up breaking into 16 dependent branches.   Yeah it’s annoying to be told to have to do something like that, but it did improve things.

Just be very careful when you rebase, use --update-refs to maintain your sanity and avoid redundant merge conflict resolutions, and be sure you all have an agreed upon strategy for the merging (merging dependent branches top down, or merging bottom up where you merge the branch closest to the feature branch into the feature branch and rebase everything else above that atop the new feature branch).

We aren't optimizing for ease of review now are we?

You probably should if you want to make sure the review is accomplishing what it is meant to.

Theres other considerations sure and you may occasionally see multiple thousand line PRs in some tricky situations, but between using feature branches with multiple dependent branches and using feature flags it’s often doable.

If you go the feature branch + multiple dependent branch route be sure to have one big unified PR before the feature branch goes to master, which would be a huge PR, but the reviews will have hopefully seen it all before piecemeal.

I am the only developer in our small team capable of writing this full stack style feature 

Ideally a team would have multiple people capable of doing this.   If that isn’t the case, how much good are the reviews going to be by to do?   Smaller PRs might help break it down so the other team members better understand what’s going on.”

and it is critical that this goes out once and smoothly.

Then you need reviews from people who aren’t simply having their eyes glaze over at a huge PR, and going through the motions of review.   Reviewers surrendering to the weight of a massive PR is not really review.

So again, my question, is a large front and back PR 5k LOC each really worse than 20 small ones that do not stand on their own for a brand new feature introduction?

In my experience yes.

At first when my manager told us he wanted smaller and more branches rather than fewer and bigger branches for review sake I was initially skeptical, but it was better.

Remember, review isn’t some superficial ceremony or ritual for merging code.   Reviewers need to try and really understand what’s going on or they’re not doing their job.   And that’s harder with massive PRs.

[u/Lotton → u/[deleted]]

You're not thinking minimal enough. You have to start thinking of mvp "to set us up for success" so let's say you build page. The design of the page would be the first PR then each different functionality on the page would be a separate PR. We need to break it down into these pieces so we can tell where the issues arise

[u/flerchin → u/[deleted]]

It can be broken up and sent incrementally. How're you gonna backout your chungus?

[u/flerchin → u/[deleted]]

Releasing everyday all day forces you to reduce the ceremony. Then adding onto your mvp is just like how you released the mvp.

[u/Stunning_Ride_220 → u/[deleted]]

Originally the term MVP did not refer to "the minimum usable product/feature iteration"....just saying

[u/Stunning_Ride_220 → u/[deleted]]

In it's original (Lean) meaning it was defined from the developer teams point of view:

It's the minimum version of a product which allows the team to gather just enough information to approach the next development iteration.

https://leanstartup.co/resources/articles/what-is-an-mvp/

But I assume most people just read the Wikipedia description and do not check or think the original sources.
There it speaks about "just enough features...to be usable by customers" which is way more than the above definition.

If I remember correctly, Eric Ries gave an example in his book about
a website just having like 3 linked pages with rough feature descriptions
and the team just measuring how much people are interested in each. (which would be a MVP in the original sense, but surely not suffice the Wikipedia description)

[u/Sea-Astronomer75 → u/[deleted]]

I do think large merge requests are kind of inevitable when working on something from scratch, but 10k loc is still excessive and won’t be reviewed well. Putting it up for review is more performative at that point because no one is going to spend an entire day thoroughly going over one merge requests

[u/bwmat → u/Sea-Astronomer75]

Why not though? I've spent hours and even days over multiple cycles reviewing a PR before

[u/prams628 → u/SuitableDragonfly]

Or you know, let the code freeze happen and say, ahh just saw it now. Was busy with work. What are they gonna say? Don’t work. Win win either way

[u/ramdomvariableX → u/SuitableDragonfly]

LOOK, who's being very responsible..

[u/Still_Bit_7527 → u/SuitableDragonfly]

20 PRs? So you are one of these asholes..

[u/SuitableDragonfly → u/Still_Bit_7527]

If you think ensuring code quality makes someone an asshole, I pity anyone who has to work with you. 

[u/Still_Bit_7527 → u/SuitableDragonfly]

I pity anyone who has to work with pendantic people who offer stress and hostility for no practical gain, but only their pride. PRs have never and will never ensure code quality unless you are FAANG

[u/SuitableDragonfly → u/Still_Bit_7527]

The practical gain is that your code actually works. And no, you don't have to be in FAANG to write code that works. Where you work in fact has nothing whatsoever to do with whether your code works or not. If you can't be bothered to actually do your job correctly, I suggest you pivot into burger flipping or selling pictures of your ass on OnlyFans, those jobs don't require any skills, so they should suit you well. 

[u/Still_Bit_7527 → u/SuitableDragonfly]

Your code should work even without PRs. YOU are responsible for its quality.You are yapping bs and claiming I said that code quality doesnt matter, instead of "being pedantic about code reviews doesnt matter".

I am not one of these pedantic 25yo FAANG spergers that will make people submit 20 PR and waste the entire day killing their braincells on submissions because they read it in a book. A SWE is paid a lot to waste their time like that. They can submit a PR, I will take a quick look, and it is on their heads to make sure the code is tested, instead of wasting both our time on meaningless rituals.

What do you think happens in a nornal company where SWEs are drowned in tasks and have 0 capacity left? Do you think anyone sees 20 PRs and will actually read through all of them carefully?

[u/SuitableDragonfly → u/Still_Bit_7527]

Nothing you write is going to work the first time. Doesn't matter how many times you say it should, that doesn't change the fact that it won't. Working code doesn't come from some sort of innate personal genius, it comes from multiple people reviewing it and testing it. 

If it costs you any brain cells at all to click a button on github, or you can't figure out how to break work into normal sized tasks, you better start looking at openings at McDonald's. 

Being constantly drowned in tasks is not a normal thing that happens at normal companies. At normal companies, you schedule the amount of work you can complete, and then you do that work. If you scheduled 20 PRs worth of work when you know you don't have the time to finish that, you're doing your job incorrectly. 

[u/Still_Bit_7527 → u/SuitableDragonfly]

Maybe your code doesn't work for the first time. Mine does (when it goes to PROD). Without PRs, because we have silent agreement to just accept everything. People who have to meet deadlines dont have time to carefully inspect whatever shit others write. And why would I break down tasks? that is your wish not mine. Integration with a new external platform is 1 task. 1 PR. 1 test phase. Maybe 3000 LOC who knows. When I deploy it, it must work. If I fuck up, it's on me. I wont waste another persons time who has to meet their own deadlines to find bugs in my code that I should have found.

And why would I even break it down? If the feature is not complete, what am I merging here? Sleeping code? You come and tell my manager to initiate a test phase assigning testers to 1 week of evidencing for 100 LOC of useless sleeping code and then tell me how it went

[u/SuitableDragonfly → u/Still_Bit_7527]

If you think your code works the first time, that just means you never actually test it to find out if that's true or not. 

You break down tasks so that they are manageable and so that you can make consistent progress. Maybe you wouldn't have so much trouble with deadlines if you actually knew how to manage tasks? If none of your teammates are ever looking at each other's code, you're not actually a team, you're just a collection of egotistical morons who are all working on their own untested hobby projects. 

Again, if you don't know how to break down big projects into manageable tasks, you don't know how to do the job. 

[u/Still_Bit_7527 → u/SuitableDragonfly]

Manageable how?

I write code, I commit code, I test it, I make ONE big PR for ONE feature, someone reviews it, if it passes it starts an entire chain of beurocratic bs to get release approval. If not, for external reasons, I revert ONE commit. If yes, there is a 3 week testing phase, and if all is good it goes to prod.

What exactly is there to break down here? Why? Ok I break integration down like this

Task 1 = add the external interface DTOs. What is there to review? Why would I bother an engineer swimming in tasks for this? The change offers no value. And then what are the testers supposed to test? How do I revert 20 PRs if the release is cancelled because of some bs?

[u/WernerderChamp]

Reply after 15 mins "sorry, had to do this important task first".

Not even a lie. I am not waiting all day for your silly PRs.

[u/saevon → u/WernerderChamp]

"I was on my coffee break, celebrating the successful upcoming code freeze"

[u/returnFutureVoid → u/saevon]

Taking a dump.

[u/WernerderChamp → u/returnFutureVoid]

Sometimes the best thing to figure out the solution for a problem.

[u/returnFutureVoid → u/WernerderChamp]

If by sometimes you mean always yeah.

[u/screwcork313 → u/returnFutureVoid]

Boss makes a dollar,
I make a dime.
That's why I'm only reviewing your ugly "poop request"
One ass-load at a time.

[u/Ok_Equipment8374]

Rejection: please include your cahtbot logs so I can get some form of understanding

[u/Outrageous-Machine-5]

You can reject PRs

[u/Cylian91460 → u/Outrageous-Machine-5]

You can also say that you won't be able to read before code freeze and that it have to wait

[u/LawfulnessDue5449 → u/Cylian91460]

Time to implement last call for code

[u/QuestionableEthics42 → u/[deleted]]

Where do you work?!? That's insane.

[u/AwkwardWillow5159 → u/QuestionableEthics42]

Yeah, I think his experience is the outlier here

[u/MrDilbert]

That's OK, PRs are asynchronous, that one can stay open until after release.

[u/swierdo]

Just make it their problem again.

"Hey, your PR is a bit large and I'm having a hard time wrapping my head around all of it. Can you walk me through the changes? Once we've done that I can properly review it. I'm available whenever, thanks!"

[u/brixon → u/swierdo]

20 years ago our code reviews were actual meetings where the author had to walk through the changes and defend their changes in front of multiple developers

[u/InexplicableBadger → u/brixon]

True, but the world changed and now you can back out your failed code in 5mins on a CI/CD pipeline, so speed became more important to most businesses than quality and accuracy. Also you don't have to do it in 640k, which famously should be enough for anybody.

So I don't have to defend my pull request in a meeting, but I do have to defend it with the project team and it has to work correctly, but a certain level of imperfection and wastefulness with memory is acceptable.

[u/Hutius1 → u/InexplicableBadger]

I much prefer this option, as having performed both. Prove competence over getting a few people to be confident in my work. Taking the ego and relative subjectivity out of it with tests is net good. There is the odd item of performance that comes up, but that's a scope parameter adjustment from there.

[u/FattySnacks → u/swierdo]

This is way more polite than the scenario calls for

[u/bwmat]

I'll start reviewing, not gonna hurry though

[u/requion → u/bwmat]

Would be irresponsible to rush a review. Especially with so many changes.

[u/madcow_bg → u/requion]

Especially before code freeze...

[u/cosmicomical23]

A PR with 10k lines is anyway impossible to review. You are basically asking me to approve it blindly.

[u/frogjg2003 → u/cosmicomical23]

I just made a massive PR with 5 digit number of lines changed. Granted, it was almost entirely deleting config files and deprecated code. So the actual changes were only about a dozen lines.

[u/cosmicomical23 → u/frogjg2003]

Ok there are some obvious special cases. For instance wide sweeping changes where you are doing one thing to many files are ok. Especially if you are not mixing in random other changes, that becomes mostly a matter of passing the tests, rather than needing a real review.

[u/Fenzik → u/cosmicomical23]

What do you mean? You just do /review Then copy/paste the output into GitHub

[u/JustExpect → u/Fenzik]

lol

[u/Cylian91460 → u/cosmicomical23]

Skill issue

[u/_Lost_as_Hell_ → u/Cylian91460]

I can review 10k LOC but why should I? I can guarantee that PR is easy to break into multiple small PRs.

[u/bwmat → u/_Lost_as_Hell_]

I don't think they always are

Like, I'm thinking back to a recent task I had to add the capability to interrupt execution to return output parameter data in a streaming fashion instead of forcing buffers for them to be bound upfront (An ODBC 3.8 feature for the curious) 

It involved replacing one of our oldest and most important interfaces with a new one which would enable this, and so rewrite all the logic on top of it to use the new one, which completely changed the flow of the code and the logic of how the execution was carried out. 

I spent a couple of months doing this, along with updating all the other places that used the old interface (we have multiple `front ends' along w/ ODBC), including adding wrappers for the old interface which translated it into the new. 

I can't imagine how I could split this change up more than I already did (1 per front-end) without making the codebase a Frankenstein of feature toggles, and no real way of testing the new logic until it was all done anyways, as it was basically replacing the old wholesale. 

IMO in this kind of situation, splitting up the PRs would make it even harder to review, since you would lack context in the initial ones which only appeared in the later ones. 

The whole 'their eyes will glaze over' IS a 'skill issue' (or even just laziness) 

[u/_Lost_as_Hell_ → u/bwmat]

Yeah I agree these situations can happen. In this case, first prepare a small tech proposal doc. Get aggreement on that first. And then after raising PR, ask reviewer if call is required to make review easy. Take all suggestions, and do careful self-review of new change. Then only ask for review again.

[u/Romeo3t → u/bwmat]

I wonder if stacked PRs come to the rescue here. Consider:

1. Work on the problem as you did
2. When you're at the end break it part into manageable chunks that are reviewable
3. You don't necessarily even have to make those chunks compilable or passing tests on their own. Each chunk should just implement something that is easy(ier) for your peers to wrap their head around.
4. Have someone review the stack understanding that the middle of the stack is unfinished code and the later PRs in the stack will bring more completion.
5. Boom, reviewers have easier chunks to review and you can still work and merge/land the entire thing as part of a massive feature branch with multiple commits.

[u/bwmat → u/Romeo3t]

Maybe...

I think it would make it harder for me as a reviewer, personally, but maybe I'm unusual. I find having the full context to be best, even if there's a lot of changes

[u/cosmicomical23 → u/Cylian91460]

It is a devex issue, not skill. I don't have to show you my cv, to tell you your take is very old fashioned and slightly toxic. Which I believe is at least part of the reason for the many downvotes.

[u/Cylian91460 → u/cosmicomical23]

It is a devex issue, not skill

Not knowing how to split task through time based on your own enjoyment is a skill issue.

[u/qxxx]

happens nowadays all the time at my company.. and boss is forcing us to review this ai slop quickly because it needs to be released in 1 hour... fuck this

[u/JustExpect → u/qxxx]

Actual truth. Completely screw QA and sprints.
Every day is a sprint. Tests are made by the AI that wrote the code. It tested itself and says it works, send it!

[u/Bomaruto]

Let it sit until code unfreeze and deal with it then.

[u/AlwaysHopelesslyLost]

"code freeze" doesn't mean "don't write or push code changes." 

It means "don't push code changes to production."

[u/JustExpect → u/AlwaysHopelesslyLost]

Yes lol exactly

[u/az987654]

No one says you have to read or accept a PR

[u/AnEvanAppeared → u/az987654]

No one says you have to read to accept a PR

[u/Dry_Debate_8514]

Find a small error. Ask for it to be fixed while you are reviewing the next part until the 20 minutes are over.

[u/traplords8n]

Jokes on you, my lead dev is scared of git and refuses to do the classic PR workflow.

We literally edit with a merge program, file by file (or directory by directory if we're feeling productive & ballsy)

I tried advocating for using git, but he's not having it lmao. Programming in a small business is wild y'all

[u/neondirt → u/traplords8n]

Yikes! I can't even remember a time before source control...

[u/traplords8n → u/neondirt]

I used to be cool with it because I didn't want to learn git, but now I use it for side projects and fucking love it... lol

[u/thanatica]

A release stop is one thing. Those are useful during the holiday season.

A code freeze feels like those releases are being built from the master branch 😬

[u/Faangdevmanager]

As if I’d review a 10k line merge request. Split it or GTFO

[u/clauEB]

Reject. It's not your problem this person has no common sense.

[u/WhoKnewSomethingOnce]

That's how you ensure sufficient work for bug fixing week.

[u/Zeratas]

Which is when you say there's no way I'll be able to review this in time, even with the usage of large language models. Please break it up into multiple merge requests with each car getting a specific piece of functionality.

[u/nicman24]

no

[u/rage4all]

"Just cleaning up some TD" with hundreds of unrelated microchanges...

[u/Psychoboy]

That's when say no you wait

[u/ScienceAlien]

Next sprint

[u/oretoh]

First time I heard about a code freeze in my 12 years in development. People are doing that now?!

[u/null_ghost_00 → u/oretoh]

Some companies have deployment freezes where certain points of the year you dont allow releases to production. Thats business operational decision. If its a code freeze where you're not allowing merges then you probably have a bad code management workflow.

[u/FlakyTest8191 → u/oretoh]

Makes sense in some environments. When there is a heavy process for a release because of compliance or safety, releasing each tiny change individually makes no sense, and you need a cutoff.

[u/ngqhoangtrung → u/oretoh]

Very necessary for mobile, especially when QA is going through regression test

[u/According-Relation-4 → u/oretoh]

Yes it’s ridiculous

[u/private256]

And then when you review it, they have an LLM respond to your feedback.

[u/LegitimatePants]

We CaN jUsT reVerT iT if iT cAuSeS a pRoblEm 

[u/sneradicus]

I would legit just say I was assigned high priority work that will require my full attention.

[u/Multidream]

The power of “no.”

[u/ICBanMI]

My co-worker was the king of trying to check in broken code right before the code freeze. He'd check it in an hour past, ship it to the customer, no check out, and then it'd crash at startup when the customer finally checked it, or how it'd be super obivious the helper window was completely upside down for the feature they requested? And it would take him 3 more builds to them to get it oriented upright?

[u/hraath]

The responsible thing to do is say this aint gonna happen in 15 minutes and ask them to split it lol.

[u/huuaaang]

I'd reject it even if it wasn't just before a code freeze.

[u/s0litar1us]

No. This is garbage and it came in too late.

-Linus

[u/Lachtheblock]

Option 1, code doesn't get merged. Option 2, release gets delayed. Product managers get to choose, but don't get it both ways.

I'm sure as hell not putting my name on something that I haven't approved because others can't time manage. It almost guarantees more work in the long run.

[u/Lane-Jacobs]

not super related but is anyone else getting sick of being handed AI documents and AI code and being asked to review it? it contains mistakes and then they get pissy when you call it out. currently have a senior dev doing this :)

[u/NormanYeetes]

straight up the healthy answer to that would be "i ain't doin allat"

[u/tbhaxor]

Worse, I got vibe coded changes in 200+ files and it on Friday 30 mins before my off hours. I had to take this task because sat-sun was the delivery.

[u/zalurker]

Reject and raise it with the team lead. Mr Swifty will have to explain himself at next code review.

[u/Putrid_Sandwich_5805]

First message: You need it now or after code freeze, with a cc to their manager

[u/Smalltalker-80]

I'm getting violent thoughts..

[u/sirdmz]

cool, this is very simple. it takes longer than 15 minutes to review the 10K lines.

sounds like these changes aren’t making it into this version, unless it meets the code freeze requirements, which it won’t.

[u/Sahnisani]

Co-aurhored by Claude

[u/TallGreenhouseGuy]

LGTM 👍👌

[u/Repulsive-Hurry8172]

Is it "looks good to merge" or "looks good to me"? IDK what LGTM means 

[u/djdanlib → u/Repulsive-Hurry8172]

Or "Let's get this merged"

[u/thanatica → u/Repulsive-Hurry8172]

Let's Get Tacos, Mate

[u/No-Plan-7070]

LGTM

[u/dmullaney]

LGTC

(looks good to Codex)

[u/Prod_Meteor]

So, if not 15mins before freeze, 10K lines are ok? Also.. have you heard AI? 😄