974
Review Please (programming.dev)
submitted 7 months ago by JPDev@programming.dev to c/programmer_humor@programming.dev
112 comments fedilink hide all child comments
you are viewing a single comment's thread
view the rest of the comments
[-] tatterdemalion@programming.dev 10 points 7 months ago

This does seem like a potential issue if the PR is itself implementing more than one vertical slice of a feature. Then it could have been smaller and there might be wasted effort.

If the patches are small and well-organized then this isn't necessarily a bad thing. It will take more than one day to review it, but it clearly took much more time to write it.

[-] Blamemeta@lemm.ee 5 points 7 months ago

True, but at the same time its very possible to go too small. A bunch of one line code reviews can really slow progress easily.

[-] magic_lobster_party@kbin.run 5 points 7 months ago

Stuffing multiple tasks into one PR is often bad.

  • It’s harder to review. As a reviewer it’s difficult to know which code change is related to which task.
  • It’s harder to verify. Did you really test every change you made?
  • You might end up with a “hostage” situation. There might be a few code changes in the PR that looks good and is really wanted, but other code changes in the same PR of lower quality. As a reviewer, should you just let these lower quality code changes slide so you can bring in the code change you really want? Probably not, but you’re going to let it slide either way.
[-] jjjalljs@ttrpg.network 2 points 7 months ago

You might end up with a “hostage” situation.

Reminds me of a guy I used to work with.

He'd have like a pretty normal PR to do something like change a field to be read only in some API. But then snuck into the pr there'd also be something like "change application to run locally different than prod for entirely selfish reasons" or "lower global coverage requirements" or "disable type checking in this whole folder"

[-] magic_lobster_party@kbin.run 2 points 7 months ago

I also worked with a guy like that. Impossible to predict what he was going to do in his next PR. Always a nightmare to review. Also exhausting to argue with, so let some things slide because I was so done dealing with his bs.

[-] jjjalljs@ttrpg.network 2 points 7 months ago

This guy was also difficult to argue with. He was always professional, which I guess is worth something.

One time he tried to remove all the types from a bunch of code "because they were all going to change later." I refused to allow this. We went back and forth in the comments for hours. I think eventually the lead or boss got involved. Thankfully people sided with me.

His "later" project that was allegedly going to change all the things was rejected by the team. The code in question is still used and properly typed a year later.

[-] tatterdemalion@programming.dev 3 points 7 months ago

Right but it's pretty rare that a tiny PR actually accomplishes a valuable user story.

So my point is just that lines of code is mostly irrelevant as long as it's organized well and does no more than necessary to accomplish the agreed upon goal.

this post was submitted on 29 Mar 2024
974 points (98.3% liked)

Programmer Humor

19594 readers
822 users here now

Welcome to Programmer Humor!

This is a place where you can post jokes, memes, humor, etc. related to programming!

For sharing awful code theres also Programming Horror.

Rules

founded 1 year ago
MODERATORS
Feyter@programming.dev
anzo@programming.dev
BurningTurtle@programming.dev
pylapp@programming.dev