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.

[–] GalacticGrapefruit@lemmy.world 4 points 16 hours ago* (last edited 16 hours ago) (3 children)

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.

[–] JordanZ@lemmy.world 3 points 14 hours ago

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.

[–] EowynCarter@lemmy.world 3 points 14 hours ago

pro tip for next time : make multiple smaller PR.

[–] PolarKraken@lemmy.dbzer0.com 3 points 16 hours ago* (last edited 16 hours ago)

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).

[–] BlackRoseAmongThorns@slrpnk.net 5 points 1 day ago* (last edited 1 day ago)

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.

[–] onlinepersona@programming.dev 2 points 1 day ago (1 children)

> 1k lines = LGTM

[–] ThirdConsul@lemmy.zip 5 points 1 day ago (1 children)

> 1k lines in internal tool = idc, do what you want.

> 1k lines in critical path = lol no.

[–] onlinepersona@programming.dev 11 points 1 day ago (2 children)

Who cares? It's company code. They want AI, they get AI 🤷

[–] ThirdConsul@lemmy.zip 6 points 23 hours ago

I get the blame and I have not lined a new remote job yet. So there.

[–] SleeplessCityLights@programming.dev 1 points 17 hours ago

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.