this post was submitted on 28 Feb 2024
1466 points (99.7% liked)

Programmer Humor

32448 readers
957 users here now

Post funny things about programming here! (Or just rant about your favourite programming language.)

Rules:

founded 5 years ago
MODERATORS
 
(page 2) 11 comments
sorted by: hot top controversial new old
[–] [email protected] 52 points 8 months ago (3 children)

uggg. Another multi thousand line PR. Again.

I'll leave it to tomorrow.

Tomorrow: fuck this. Ive got shit to do.

load more comments (3 replies)
[–] [email protected] 26 points 8 months ago* (last edited 8 months ago) (6 children)

As a senior at my last big company job, basically all I did was conduct meetings and do PRs. It's such a grind.

My opinion now is that most PR is worthless anyway. Most people give, at best, a superficial skim for typos, lack of comments, or other low-hanging replies (that usually, really, a static checker or linter should be dealing with).

Reading the code base in little chunks like that doesn't give you proper context for the changes you're reading. Automated unit and integration tests would be better for catching issues like that, but of course then who is reviewing and verifying the tests? Who's writing them for that matter?

Ideally, pair-programming or having extra people on projects to create knowledge redundancy would help. But companies want to replace juniors with AI now, so that's not looking good. Senior devs and architects might know the major pieces of much of the code, but can they "load it into working memory" sufficiently to do a quality PR that will catch something the tests didn't and QA wouldn't? Not in my experience.

I think the best actually-implementable solution for most teams is to get rid of PR expectations and take a multi-pronged approach to replacing that process.

  1. use tooling to check for and fix basic stuff. Use a linter, adopt a code standard, get a code formatting tool that forced adherence to the standard and run it on every PR.
  2. Unit tests if you got them, start if you don't. You don't need 90% code coverage, just make sure critical paths are covered.
  3. Turn one of your useless meetings into a code review session. Each week/sprint, one Very Important Code section is presented by the developer that works on it most or that last changed it. This helps the whole team learn the code base, gets more eyes on the important stuff regularly, and enforces not just a consistent style but a consistent approach to solving and documenting problems.
  4. PR (and the github PR approval stuff or its equivalent for you) should be streamlined but preserved. Do have a second person approve changes before merging, just to double check that tests have finished and passed and all that. If your team is so busy that no one ever approves PRs then allow self-approval and be done with it. This will make regular code review very important for security and stability, since any dev could be misbehaving unseen, but these are the trade-offs you make when burning out your team is more important than quality.
[–] [email protected] 14 points 8 months ago

I caught a junior trying to reimplement an existing feature, poorly, in a way that would have affected every other consumer of the software I'm a code owner on a week or two ago. There's good reason to keep them around.

PRs suck to do, but having a rotating team of owners helps, and linting + auto formatting helps with a lot of the ticky tacky stuff.

Honestly, the worst part is "newGuy has requested your review on a PR you requested changes on but he hasn't addressed" that'll get you in the ignored pile real quick.

load more comments (5 replies)
[–] [email protected] 61 points 8 months ago (7 children)

Ughh I'm currently waiting on a review and I've pinged people multiple times but nothing. It's blocking all my work for the rest of the week.

[–] [email protected] 5 points 8 months ago

I'll review it

[–] [email protected] 15 points 8 months ago
[–] [email protected] 48 points 8 months ago (2 children)

Be sure to call out in standup

[–] [email protected] 0 points 8 months ago (1 children)

And actually stand up. Otherwise it defeats the purpose.

load more comments (1 replies)
[–] [email protected] 94 points 8 months ago* (last edited 8 months ago) (9 children)

Me: "So, I completed this time critical task a week ago, had it QA tested, and it's been awaiting approval since Tuesday. I've posted my PR with links in the dev chat, I've pinged each of you individually each day as well. It is still awaiting approval before I can merge and pick up a new card from our backlog that is dependent on these changes. If literally anyone has the bandwidth to do this review, please do. I'll post the link here again as well, to make this super convenient for you all, as well as the Jira card for reference, and the changes and requirements themselves are extremely straight forward. It should only take 5-10 minutes, tops. And I will be sitting here useless until it is done. Somebody, please, for the love of god..."

My team: crickets

Scrum Master: "Thanks for the update, kryptonianCodeMonkey... next up is..."

[–] [email protected] 21 points 8 months ago (1 children)

That about sums it up. Even escalated to my boss at this point, still crickets. It’s not even a big or complex PR.

[–] [email protected] 33 points 8 months ago (4 children)

Sounds like paid time off to me!

load more comments (4 replies)
load more comments (8 replies)
load more comments (4 replies)
load more comments
view more: ‹ prev next ›