Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

PR process killing morale and productivity

blackentropy.com

Post reply on HN
Print view View original

81–90 of 224 posts

Re: PR process killing morale and productivity

#81

Post by dmurray »

lkrubner wrote: post #58

When it comes to code reviews, the return on investment faces the Law Of Diminishing Returns. While many of the comments made in code reviews might be interesting, they are not so interesting that they pay for themselves. If you put some dollar value on the time invested, you'll find that the vast majority of this process is simply burning money. And not only money, but also, as this article says, morale. A curious f…

> I've come to a similar conclusion. Do not write a comment in a code review, instead, write a test that would catch whatever danger you want to warn about. And if you cannot find a way to express your concern as a test, the return-on-investment of worrying about that concern is probably zero, so we should ignore it.

What if your concern is "this approach makes the code difficult to test"?

Re: PR process killing morale and productivity

#82

Post by johannes1234321 »

herpdyderp wrote: post #39
barbazoo wrote: post #5

> I’ve recently come across a discussion where a new developer joined a team and faced over 300 PR comments on their first contribution. Most of it was stylistic nitpicking. This isn’t just unproductive, it’s outright toxic. For me this says more about the company culture than any inherent flaws with the code review process.

This kind of feedback has largely been solved, for me / my team at least, with linters and formatters (as mentioned in the article). So I'd say it is still a reflection on the code review process being broken.

There is a lot of stuff a linter can't do, which however are stylistic choices some people care a lot about. Naming being a big one. Which terms to abbreviate how etc. Also there are a lot of things where automatic highlighting is possible, but any rule needs exceptions which a re subjective (nesting, usage of "raw" looos, early exit, ...)

Trying to get some consistency there can be good. And teaching new team members the "habits and traditions" may simplify long term maintenance.

However could review tools aren't a good place for distinguishing between "hey, this is good, but I'd like this slightly different" from "there is an actual issue" but that has to be solved somehow by communication. Which often isn't the best skill for engineers.

Re: PR process killing morale and productivity

#83

Post by t-writescode »

skydhash wrote: post #78
BurningFrog wrote: post #53

Earlier quoted context omitted.

At my last job, the rule was if the PR is big enough, get on a call and go over it in person. ideally with more than one "reviewer", to break stalemates. Worked really well! I used to be that introverted guy, but a few years of pair programming completely cured that, and I'm now very comfortable discussing design in detail and at length, in a (if I may say so myself) friendly and constructive way. If you've never dis…

A PR big enough should probably have its changes implemented and reviewed unit by unit on a feature branch before its creation. Or it should be an already approved work (refactoring, reformatting,...)

And if it's refactoring / reformatting, then it should be pair-programmed to validate that exactly what was said to have been done was done and the pair programmer should confirm such in the PR.

Re: PR process killing morale and productivity

#84

Post by kleinsch »

herpdyderp wrote: post #43
eddd-ddde wrote: post #37

Earlier quoted context omitted.

It also requires good tooling and some experience. GitHub absolutely sucks at making multi-commit features. At least last time I used it. And getting people to actually make smaller commits and reviews is incredibly hard.

Can confirm GitHub's process is poor for small PRs (if that's what you mean by "multi-commit features"). My team has mostly gotten around this with custom CLI tools (for pushing small PRs chained together) and web apps (for concisely viewing your code review status, both giving and receiving).

I’m a huge fan of https://github.com/VirtusLab/git-machete

Re: PR process killing morale and productivity

#85

Post by nmstoker »

This is really important to get right. There's a balance as you want to avoid introducing crap but a little restraint and wider awareness makes a huge difference.

I recall being significantly put off after making my first significant contribution online to a non-trivial repo.

I was a volunteer and had been making various very modest contributions (ie simple fixes) along with efforts around docs and answering questions from users. A major contributor was known for being cantankerous. He knew what I was bringing in general, and seemed grateful yet didn't hold off with pretty forceful feedback on my first attempt at something more substantial - I had taken a day off to get it in good shape, I knew it was solving something with broad use (ie not just for me!). There were a litany of points. I fixed several but each time there would be more, it was like water on Gremlins! I gave up and stood back for a bit. Ironically it would have been trivial for him to take the code in but it sat forever and the whole thing was a waste of time but I guess I learned a few things!

I try to encourage standards but not officiously!

Re: PR process killing morale and productivity

#86

Post by t-writescode »

rileymat2 wrote: post #68

Part of me wonders if the real issue is we don't have an author/editor type system. If the reviewer/editor could just make the nitpicky changes in about the same time it takes to call them out, the relationship might be much more healthy. Things could go back and forth in a much more healthy way.

Jetbrains SpaceCode (rip), Github and Gitlab all provide that sort of system. I imagine the other ones do, too.

Re: PR process killing morale and productivity

#87

Post by patrickhogan1 »

I’ve had a PR into an open source Anthropic library open for almost 2 months. It passes all of their rules, fixes an obvious bug that causes functional issues, passes all tests, and has a description that is descriptive with a complete user story explaining the functional issue with images. I really like using Claude and I like Anthropic’s mission but it definitely kills morale to fix something and be put into PR purgatory.

Re: PR process killing morale and productivity

#88

Post by powersnail »

steveBK123 wrote: post #74
powersnail wrote: post #71

I don't know the original story, but 300 hundred PR comments on a first PR (or any PR for that matter) is insane, and my guess is that somebody on the team is having an argument back-and-forth in that PR, rather than focusing on whether the code should be merged. If that happens to me, I would kindly invite them to talk about it elsewhere and give me pass, because the fact that you guys are still debating it, means t…

I was once on a team where this was not unusual, and the problem was the tech lead was the one engaging in 300 PR comment arguments. This would result in stuff sitting in purgatory forever. The funniest part was this was, in 20 years, the absolute worst codebase I'd ever seen, so he wasn't even maintaining some high standards in the codebase by doing so.

Sounds about right. If so many technical decisions are made by a single individual while reviewing PR, rather than having a proper standard agreed upon before hand, it's no wonder that it grows into a horrible code base.

Re: PR process killing morale and productivity

#89

Post by grecy »

PhilipRoman wrote: post #29
barbazoo wrote: post #5

> I’ve recently come across a discussion where a new developer joined a team and faced over 300 PR comments on their first contribution. Most of it was stylistic nitpicking. This isn’t just unproductive, it’s outright toxic. For me this says more about the company culture than any inherent flaws with the code review process.

IMO the best way to handle stylistic choices (if at all) is to have the reviewer just adjust it themselves. Ping-pong with comments like "add empty line" is a waste of time for everyone.

> Ping-pong with comments like "add empty line" is a waste of time for everyone.

And any manager that lets it happen without severe reprimands to the time-waster isn't worth working for.

We've all got way more important things to be spending our time on.

Re: PR process killing morale and productivity

#90

Post by watwut »

PhilipRoman wrote: post #29
barbazoo wrote: post #5

> I’ve recently come across a discussion where a new developer joined a team and faced over 300 PR comments on their first contribution. Most of it was stylistic nitpicking. This isn’t just unproductive, it’s outright toxic. For me this says more about the company culture than any inherent flaws with the code review process.

IMO the best way to handle stylistic choices (if at all) is to have the reviewer just adjust it themselves. Ping-pong with comments like "add empty line" is a waste of time for everyone.

IMO, the best way is to have automatic formatter shared by team.
Post reply on HN