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.
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.
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.
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.
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.
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
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.
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
You are not logged in. However you can subscribe from another Fediverse account, for example Lemmy or Mastodon. To do this, paste the following into the search field of your instance: !programmerhumor@lemmy.ml
Post funny things about programming here! (Or just rant about your favourite programming language.)
Rules:
Posts must be relevant to programming, programmers, or computer science.
No NSFW content.
Jokes must be in good taste. No hate speech, bigotry, etc.
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.
As a required reviewer on a lot of things, I envy that bear so, so much.
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.
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:
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
This comment seems like a lot of work to read, I’ll pretend I didn’t see it
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.
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
God damn it.
Certain this works 100% in the wild. Main issue will be trying to SSH into the server, unless you can borrow their hotspot.