"So, Daywim. Why did you let this obviously aweful PR pass your desk causing so much trouble for our company? I'm afraid we have to let you go because of this questionable performance." - Corporate

[–] EffortlessEffluvium@lemmy.zip 8 points 2 hours ago (1 children)

What part of forever holiday did you miss?

[–] einkorn@feddit.org 2 points 2 hours ago

I interpreted it as "Holiday that lasts forever because the company can't work anymore" but I guess it is meant to mean "Holiday that lasts forever cause you got fired"?

[–] ATPA9@feddit.org 3 points 1 hour ago

"Looks like I overlooked something in this 6k PR full of im meaningless dribble. Why don't you ask the person who comitted the code how he overlooked this bug. Its his respinsibility"

Just throw the the slop creator under the bus.

[–] flb@reddthat.com 8 points 3 hours ago (3 children)

Our PR checks auto reject the PR if it has 1k changes

[–] Pika@sh.itjust.works 4 points 2 hours ago* (last edited 2 hours ago) (1 children)

is it auto reject, or just doesn't auto approve and leaves it open for manual review

It seems weird that you can't do a pr at all with 1000 line changes, any moderate size feature addition could hit that mark

[–] theolodis@feddit.org 2 points 2 hours ago (1 children)

You know, you could just chunk it up in a way to keep it readable.

[–] Pika@sh.itjust.works 4 points 2 hours ago* (last edited 2 hours ago)

for a new feature request? a PR isn't a commit, it's a set of commits which would add to the line change amount.

Like even if you spread it out across 20 or 30 commits that's still going to be the same line count.

I guess you could push not yet functional or used code to lessen the line count change, but that seems in bad taste. I've always gone off the working repo should always be in build or clean state and a push or commit shouldn't break that.

[–] victorz@lemmy.world 1 points 1 hour ago

What if you remove some files? 🙃

[–] getFrog@piefed.social 3 points 3 hours ago (1 children)

Stupid question, but what happens to a rejected PR? Because features get built for a reason (there's usually a Ticket/Story for the feature that the PR adds) so do those just get closed? Or does the person have to re-write the code entirely?

I know in my team, the most I could do is tell the coworker to self-review while keeping the PR open until they change some stuff. I have never rejected a PR before because no matter how bad a PR is, it always is technically necessary for the feature.

[–] einkorn@feddit.org 1 points 2 hours ago

If a feature request requires changes of such a magnitude it is important to break them down into smaller chunks that can be reviewed either independently or sequentially.

I have never rejected a PR before because no matter how bad a PR is, it always is technically necessary for the feature.

Define bad? If the PR contains lots of unnecessary changes such as formatting or renaming simply tell the person to roll them back and come again unless they have very good reason to do so.

If the code quality is bad, well, that's why you are doing the review. If all that matters was "Does it do what it is supposed to do most of the time?" some simple unit tests would be enough. Reviewing code means making sure it does what is supposed to do and does so in an acceptable manner. Criteria can be amongst others speed, security, ease of use or maintainability.

Insecure handling of inputs? Add sanitisation and resubmit.
Overusage of resource intensive features such as database queries? Group and optimize queries and resubmit.
The codes formatting is not according to the internal style guide? Configure your damn linter and resubmit.
...

If you don't want to close PRs outright you can request new commits that fix the issues you identified, reevaluate the PR and decide again.

[–] dylanTheDeveloper@lemmy.world 9 points 6 hours ago

No Mr Bond, i expect you to approve

[–] MousePotatoDoesStuff@piefed.social 5 points 6 hours ago

LBTM, rejected instantly.

[–] skisnow@lemmy.ca 26 points 8 hours ago (4 children)

We've had a very recent uptick in engineers submitting PRs of hundreds of lines across multiple files, for Jira tickets that only asked for a one-line change. The engineers involved have been using AI assistants for nearly two years now, but there seems to have been a change in the last month or so in how aggressive the new models are at changing code.

[–] Blackmist@feddit.uk 37 points 8 hours ago

Almost as if they're paid by the token...

[–] tetris11@feddit.uk 3 points 7 hours ago

Use ponytail to keep 50 line changes to 1 line, and use rtk to save tokens.

Also use an assistant to checkout someone else's diff and review it in chunks

load more comments (2 replies)

[–] JordanZ@lemmy.world 24 points 10 hours ago (7 children)

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.

[–] GalacticGrapefruit@lemmy.world 1 points 18 minutes ago* (last edited 12 minutes ago)

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.

[–] BlackRoseAmongThorns@slrpnk.net 4 points 8 hours ago* (last edited 8 hours 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.

load more comments (5 replies)

[–] CookieOfFortune@lemmy.world 8 points 9 hours ago* (last edited 9 hours ago) (2 children)

If LLM can make big PR, LLM can split PRs

[–] ZILtoid1991@lemmy.world 5 points 1 hour ago

The goal of AI providers is to make humans unable to maintain code, so you have to rely on their expensive subscriptions and tokens.

[–] okamiueru@lemmy.world 1 points 2 hours ago

I'm not sure if you're suggesting to use LLMs to review bad PRs, or that the author should redo them. Latter, for sure. Former, sounds horrible.

load more comments