I honestly wish for a PR this size. One of the ones that came across this week was 813 commits, +17K -2K.
Of the 250 commits that GitHub was willing to show it had 35 other PRs merged into this massive one. Why they thought one giant PR was somehow better I’ll never know.
Of course…high priority, please review and merge immediately. Like guys it’s gonna take me a week to make sense of this.
Jeez. Now I feel bad. I’ve been working on a project with some new people, and I’ve never used Github before. I’m still learning the etiquette.
I made a branch, and spent a month viciously hunting every bug I could find. I don’t trust AI, so I was doing it all by hand. Dawn to dusk, I was staring at code and typing like I had a fever and the only cure was figuring out where tf that invalid scope is supposed to go.
This is my first real project with other people, so of course I’m so proud when I send the PR and it has 40,000 lines added and 60,000 lines removed. I worked really hard on it, and it sorely needed the update.
It was almost all bug fixes, the actual new stuff was about 1,000 or so lines. But what should I do in the future? I don’t wanna be an asshole, I wanna be helpful.
You could submit a PR for each bug or if there are a few bugs around the same thing then bundling those up is fine. You could have been submitting PRs every couple hours to a day for that month.
These have a lot of potential for abuse so use with caution but things that have dependencies on each other can use stacked PRs(yeah, this is GitHub specific). I’ve also found that splitting out any generated code with these is tremendously helpful at review time. Not talking about AI generated code. An example would be something like using a client generator for an OpenApi document. That way the tens to hundreds of files from the generator are in their own layer of the stacked PR and all the logic that uses it is in the other. You review each layer individually and can spend significantly less time on the generated one.
Don’t feel bad especially if it was your first time. This is my largest PR ever. It was just removing old projects from a repo though that were triggering our security/vulnerability scanning though.
Generally it’s more considerate to submit smaller batches of (self-contained!) work at a time. Sometimes the work or existing code is so interconnected that this can’t really be done, though. You mentioned spending a month, that’s also a long time in most environments to be off working on bulk changes for later review. A week is probably long enough in most cases, some orgs even prefer to push (if not review) code daily.
Don’t agonize over it though, wanting to improve your impact on others while working is the right direction to point, keep walking that way and you’ll do awesome, let it develop over time. As in, don’t let your desire for politeness slow down your work or growth too much (I do this lol, why I’m mentioning).
i’d say it’s a balance… you’re totally right that individual requests for review should be relatively small (mostly so that they can all fit in your head at once), but imo equally valid is that everything in main should be a complete feature/fix: if you were to be gone immediately after merging, would someone need to continue or revert the change? would there be unused code laying around?
this is where merge trains and a decent UI around them comes in handy: your main work is on a branch many small PRs each reviewed individually merge into that branch, and then when you’re done pretty much just automated integration tests, lint, and you’re good to merge the whole
but equally some people prefer to solve this with things like gitflow, or just not at all and accept that main is always in flux
refactor to support a new feature is also tricky: does it belong with the feature because it’s unnecessary abstraction without it? or is it its own PR because it stands in its own? and if it’s its own PR then how do you base your own feature branch on it before someone reviews and merges? how do you know you’re done without finishing? what if your assumptions are wrong and you need to try something new - just a lot of unnecessary churn and review?
dev is messy and as always LOC is a pretty useless metric… keeping things understandable is key, and somethings a 17k line PR is the cleanest way to proceed
Oh I totally agree with you. However I tried pitching the whole integration branch idea to my team and they didn’t really like it that much for whatever reason. We now just stack small feature branches on top of each other (to build the whole feature) and integrate the parts directly to master.
The review process is all wrong if something like this is ever on the table as a single PR*.
Big changes like this were made before, and knowing how to split the work (or at least trying to work it out) used to be part of the job.
Hopefully, strong unions and worker involvement can remedy this, given we change our work culture to be closer to what projects like SQLITE and FFMPEG have (noting, of course, the fact these are FOSS, and made by volunteers, yet are very dependable), slower and stable development cycle that prioritizes high quality work that people can actually depend on and trust.
As in one single PR you’re expected to read, instead of one backed by tests and the like.
We go by description of the PR. If you can briefly describe the changes you want to merge, We will consider accepting it. If you have two paragraph or more, insta close. If someone has made large changes, there is no way they can have a short description, either break it down to smaller PRs or admit you don’t know what you are doing and are trying to merge garbage.
I honestly wish for a PR this size. One of the ones that came across this week was 813 commits, +17K -2K.
Of the 250 commits that GitHub was willing to show it had 35 other PRs merged into this massive one. Why they thought one giant PR was somehow better I’ll never know.
Of course…high priority, please review and merge immediately. Like guys it’s gonna take me a week to make sense of this.
Jeez. Now I feel bad. I’ve been working on a project with some new people, and I’ve never used Github before. I’m still learning the etiquette.
I made a branch, and spent a month viciously hunting every bug I could find. I don’t trust AI, so I was doing it all by hand. Dawn to dusk, I was staring at code and typing like I had a fever and the only cure was figuring out where tf that invalid scope is supposed to go.
This is my first real project with other people, so of course I’m so proud when I send the PR and it has 40,000 lines added and 60,000 lines removed. I worked really hard on it, and it sorely needed the update.
It was almost all bug fixes, the actual new stuff was about 1,000 or so lines. But what should I do in the future? I don’t wanna be an asshole, I wanna be helpful.
You could submit a PR for each bug or if there are a few bugs around the same thing then bundling those up is fine. You could have been submitting PRs every couple hours to a day for that month.
These have a lot of potential for abuse so use with caution but things that have dependencies on each other can use stacked PRs(yeah, this is GitHub specific). I’ve also found that splitting out any generated code with these is tremendously helpful at review time. Not talking about AI generated code. An example would be something like using a client generator for an OpenApi document. That way the tens to hundreds of files from the generator are in their own layer of the stacked PR and all the logic that uses it is in the other. You review each layer individually and can spend significantly less time on the generated one.
Don’t feel bad especially if it was your first time. This is my largest PR ever. It was just removing old projects from a repo though that were triggering our security/vulnerability scanning though.
pro tip for next time : make multiple smaller PR.
Generally it’s more considerate to submit smaller batches of (self-contained!) work at a time. Sometimes the work or existing code is so interconnected that this can’t really be done, though. You mentioned spending a month, that’s also a long time in most environments to be off working on bulk changes for later review. A week is probably long enough in most cases, some orgs even prefer to push (if not review) code daily.
Don’t agonize over it though, wanting to improve your impact on others while working is the right direction to point, keep walking that way and you’ll do awesome, let it develop over time. As in, don’t let your desire for politeness slow down your work or growth too much (I do this lol, why I’m mentioning).
Lol nothing bigger than 250 lines of code goes through at our company without complaints.
i’d say it’s a balance… you’re totally right that individual requests for review should be relatively small (mostly so that they can all fit in your head at once), but imo equally valid is that everything in main should be a complete feature/fix: if you were to be gone immediately after merging, would someone need to continue or revert the change? would there be unused code laying around?
this is where merge trains and a decent UI around them comes in handy: your main work is on a branch many small PRs each reviewed individually merge into that branch, and then when you’re done pretty much just automated integration tests, lint, and you’re good to merge the whole
but equally some people prefer to solve this with things like gitflow, or just not at all and accept that main is always in flux
refactor to support a new feature is also tricky: does it belong with the feature because it’s unnecessary abstraction without it? or is it its own PR because it stands in its own? and if it’s its own PR then how do you base your own feature branch on it before someone reviews and merges? how do you know you’re done without finishing? what if your assumptions are wrong and you need to try something new - just a lot of unnecessary churn and review?
dev is messy and as always LOC is a pretty useless metric… keeping things understandable is key, and somethings a 17k line PR is the cleanest way to proceed
Oh I totally agree with you. However I tried pitching the whole integration branch idea to my team and they didn’t really like it that much for whatever reason. We now just stack small feature branches on top of each other (to build the whole feature) and integrate the parts directly to master.
The review process is all wrong if something like this is ever on the table as a single PR*.
Big changes like this were made before, and knowing how to split the work (or at least trying to work it out) used to be part of the job.
Hopefully, strong unions and worker involvement can remedy this, given we change our work culture to be closer to what projects like SQLITE and FFMPEG have (noting, of course, the fact these are FOSS, and made by volunteers, yet are very dependable), slower and stable development cycle that prioritizes high quality work that people can actually depend on and trust.
> 1k lines = LGTM
> 1k lines in internal tool = idc, do what you want.
> 1k lines in critical path = lol no.
Who cares? It’s company code. They want AI, they get AI 🤷
When you have ownership of the project you care.
We go by description of the PR. If you can briefly describe the changes you want to merge, We will consider accepting it. If you have two paragraph or more, insta close. If someone has made large changes, there is no way they can have a short description, either break it down to smaller PRs or admit you don’t know what you are doing and are trying to merge garbage.
I get the blame and I have not lined a new remote job yet. So there.