1376
submitted 1 month ago by [email protected] to c/[email protected]
top 50 comments
sorted by: hot top controversial new old
[-] [email protected] 92 points 1 month ago

We decided that everyone in the team is allowed to approve changes. If no one has reviewed your change within 24 hours you are allowed to approve it yourself. It will usually come up in the daily sync that a self approval is imminent, which usually leads to someone taking a look.

[-] [email protected] 25 points 1 month ago
[-] [email protected] 41 points 1 month ago

Surely this will just devolve into "no reviews ever".

[-] [email protected] 7 points 1 month ago

More like "Jerry reviews everything"

[-] [email protected] 4 points 1 month ago* (last edited 1 month ago)

We very seldom resort to self approvals. Everyone in the team see code reviews as important. But also that progress trumps code review.

[-] [email protected] 15 points 1 month ago

Self-approval leads to a road of sadness. For example, a theoretical company needs to self-renew an ssl cert. No problem, the cert will be stored with the rest of the secrets and retrieved in a secure way on deployment. Unfortunately if you don't store the cert key in a secure way, the deployment still works fine and you don't need to figure out the "onerous" encryption process.

So you push the private key to the company git repo, and then deploy the cert! Done and Done.

[-] [email protected] 6 points 1 month ago

We have well established ways to deal with secrets. Also, everyone is responsible enough to not self approve changes where they do things they are uncertain of.

load more comments (1 replies)
load more comments (3 replies)
[-] [email protected] 60 points 1 month ago

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] 46 points 1 month ago

Be sure to call out in standup

[-] kryptonianCodeMonkey 93 points 1 month ago* (last edited 1 month ago)

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] 34 points 1 month ago

Oh I can see your problem: everyone is still waiting for JIRA to load.

[-] [email protected] 19 points 1 month ago

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

[-] Dagnet 31 points 1 month ago

Sounds like paid time off to me!

[-] kryptonianCodeMonkey 9 points 1 month ago* (last edited 1 month ago)

If your leadership doesn't review productivity, sure...

[-] [email protected] 15 points 1 month ago

And then you have a great big fuck off stack of documentation you can slap in their face with proof that it's someone else's problem.

Unless you aren't actually pinging all the people you say you are.

[-] [email protected] 9 points 1 month ago

Hah I was dicking around but it got approved during my lunch :(

load more comments (1 replies)
[-] [email protected] 16 points 1 month ago* (last edited 1 month ago)

Every time I see these comments, I wonder if I was just lucky with my scrum masters and most actually suck, or if it’s confirmation bias. We don’t have a scrum master where I work, but my whole job as lead is keeping things rolling, and this would be just unacceptable.

[-] [email protected] 9 points 1 month ago

Yeah, I also wonder what kind of shitty culture they have in these teams? I mean, who would leave a coworker hanging like that? That's just a collective dick move.

[-] kryptonianCodeMonkey 5 points 1 month ago* (last edited 1 month ago)

My Scrum Master is nice, but her role seems to mostly revolve around enforcing documentation standards, coordinating refinements and retrospectives, tracking metrics on task completion, and maintaining our Jira board. She doesn't have a lot of involvement with the specifics of development, delegation, or how we execute our tasks.

[-] uranibaba 3 points 1 month ago

That sounds like a great boss, someone how is involved with what should be done but not always how, as long as the team deliver what is requested.

[-] [email protected] 9 points 1 month ago

"manager, person_a and person_b are the reviewers on my time sensitive PR. I'm blocked. You are aware of everyone's priorities, can you indicate prioritization of tasks and delegate how we should act?"

[-] [email protected] 7 points 1 month ago

We have big red magnets representing blocked to put on the board. We have to speak about every single blocker every stand up and what the team's path forward is to unblock the thing. If it's waiting for vendor, then that's all we can do. If the ball is in our court for any blockers, and its still there tomorrow without a really good reason, there is hell to pay.

[-] [email protected] 6 points 1 month ago

Well on the flip side, I somehow ended up doing legacy projects with a dude that has been coding for decades and is still actively developing in VB and asp.net. Weirdly, the guys not dumb - he asked me for an API and I blew his mind with generics and cut the code down by a third. I then introduced him to the concept of (primitive) components, he isn't quite sold on the importance of code reuse, but every time I delete 1k lines of old code and replace it with a 20 line function my soul grows

When we do code reviews, it's basically pair programming sharing screen... Usually we just push everything and wait for bug reports, because this crazy ass company has been using a reference book, a calculator, and hundreds of people were manually re-entering things by memory into QuickBooks until January 1st this year. They were thousands of dollars off in the second week... We thought it was a bug. It was all user errors

He's been working on this system for 15 years, I ran into a table with 126 columns the other day. Somehow, this dude manages to swim through a database with hundreds of tables and just as many triggers with rawdog sql.

It's fucking wild...I split my time between that and working on my virtual assistant that brainstorms it's own development with me, and an app that I'm trying to make into a unified fediverse client.

I know what a tight ship looks like and I push for best practices when I think there's something to gain worth the fight, but the sheer spectrum of software dev is incredible. My legacy guy told me about what's been taking all his time lately today - he has to build a system to screen scrape from an emulated IBM mainframe... And I spent my morning working on a unified activity pub interface and my evening testing my weird observation that LLMs speaking UwU seem to perform significantly better

My point being, there's a sweet spot between methodology/process, and it's very rare to hit it. And also, software dev is playing in realms beyond human comprehension, and no matter how orderly if seems it should be, every senior dev who still writes code is superstitious, and often correct to be so

Notify the people you have to notify for your blockers, then embrace the absurdity

Thank you for coming to my Ted talk

load more comments (2 replies)
[-] [email protected] 14 points 1 month ago
[-] SpaceNoodle 5 points 1 month ago

I'll review it

load more comments (4 replies)
[-] [email protected] 50 points 1 month ago

uggg. Another multi thousand line PR. Again.

I'll leave it to tomorrow.

Tomorrow: fuck this. Ive got shit to do.

[-] GeniusIsme 1 points 1 month ago

It is also a "refactoring".

load more comments (2 replies)
[-] CodexArcanum 25 points 1 month ago* (last edited 1 month ago)

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.
[-] wreckedcarzz 24 points 1 month ago

This comment seems like a lot of work to read, I'll pretend I didn't see it

[-] [email protected] 6 points 1 month ago

So you'll just hit approve?

[-] [email protected] 12 points 1 month 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.

[-] gaterush 9 points 1 month ago

I generally agree and like this strategy, but to add to the other comment about catching reimplemented code, there's just some code quality reviewing that cannot be done by automating tooling right now.

Some scenarios come to mind:

  • code is written in a brittle fashion, especially with external data, where it's difficult to unit test every type of input; generally you might catch improper assumptions about the data in the code
  • code reimplements a more battle tested functionality, or uses a library no longer maintained or is possibly unreliable
  • code that the test coverage unintentionally misses due to code being located outside of the test path
  • poor abstractions, shallow interfaces

It's hard to catch these without understanding context, so I agree a code review meets are helpful and establishing domain owners. But I think you still need PR reviews to document these potential problems

[-] [email protected] 4 points 1 month ago* (last edited 1 month ago)

these are the trade-offs you make when burning out your team is more important than quality.

Yep.

Many directors and CIOs know exactly where they stand regardin the classic value proposition: deliver something trivial before next quarterly earnings statements - at the low easy cost of losing all organizational understanding of the code base.

load more comments (1 replies)
[-] [email protected] 15 points 1 month ago* (last edited 1 month ago)

I would be fine with it if my job was just that review stuff.

But man when you are in the middle of something and you have to review something because they need it soon... It's annoying as fuck.

[-] [email protected] 15 points 1 month ago

Reviewing code is part of your job. But you also deserve uninterrupted focus time. Just block focus time blocks in your calendar and check if your peers need reviees 2-3 times a day.

[-] inclementimmigrant 13 points 1 month ago

As a required reviewer on a lot of things, I envy that bear so, so much.

[-] [email protected] 9 points 1 month ago

don't review just blind accept and merge it's more fun that way

[-] [email protected] 3 points 1 month ago
[-] [email protected] 7 points 1 month ago

Certain this works 100% in the wild. Main issue will be trying to SSH into the server, unless you can borrow their hotspot.

[-] [email protected] 6 points 1 month ago

No! Ask him to leave a rating on your encounter with him.

[-] [email protected] 6 points 1 month ago

I haven't done any serious programming in a long time. Is this mostly about corporate process and hierarchies for programming or does this apply to open source projects as well?

Seems really demoralizing putting in the work to add something to an open source project and having it waste away unreviewed and unappreciated.

[-] killeronthecorner 15 points 1 month ago* (last edited 1 month ago)

It's more about scale. Small open source projects might get one PR a month. Your average tech company is dealing with dozens of PR every single day. Review fatigue is real in these environments

[-] Thcdenton 2 points 1 month ago

God damn it.

[-] [email protected] 1 points 1 month ago

To be fair, we sometimes have to look through multiple related documentation and tickets to make sure the change was actually reviewed and approved by the necessary teams (network, security, etc.). We also have an SLA for PR reviews/approvals and some people have a habit of sending it out for approval at the last minute of the change window.

load more comments
view more: next ›
this post was submitted on 28 Feb 2024
1376 points (99.6% liked)

Programmer Humor

30512 readers
1715 users here now

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

Rules:

founded 4 years ago
MODERATORS