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

91–100 of 224 posts

Re: PR process killing morale and productivity

#91

Post by steveBK123 »

powersnail wrote: post #88
steveBK123 wrote: post #74

Earlier quoted context omitted.

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.

I think PRs are also the wrong place to litigate any of this.

Requirements, specs, architecture, etc sure.

But most systems I've worked on that were especially BAD were that way not because of some PR-reviewable syntax or style, but because the whole foundation was wrong.

The most idiomatic, consistent, linted, unit-tested, etc code in the world doesn't matter that much if the underlying architecture is a mess.

Re: PR process killing morale and productivity

#92

Post by hibikir »

OptionOfT wrote: post #80
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 is why I love things like cargo fmt / go fmt / eslint / etc. No discussions of whether if (foo) return; is valid, or we should do if (foo) { return; }

That's also why I dislike it: The mind that will go apeshit over trivial style nonsense will still provide unhelpful feedback on other topics... but it will become far less obvious that their comments should be downgraded.

Formatters also do a poor job if your language is flexible and expressive. Great for go, but if your language is the kind that easily supports internal DSLs, formatters are not even helpful at making the code regular

Re: PR process killing morale and productivity

#93

Post by EliRivers »

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.

have the reviewer just adjust it themselves

Passive-aggressive minefield. Someone will see a change done to THEIR code, without the changer even asking, and it will feel like the person who did it is a passive aggressive dickhead. Resentment will brew, tempers will be lost. It will only get worse from there. Software engineers already aren't exactly known for their humbleness and ability to swallow their ego.

Re: PR process killing morale and productivity

#94

Post by rileymat2 »

t-writescode wrote: post #86
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.

Sorry, I was not clear. I don’t mean software system, but a culture of operating in that way.

Even the language “Code Review” is not one of cooperation, but creates thoughts of a “movie review” where one rates and finds error; not collaboration and mutual editing.

Re: PR process killing morale and productivity

#95

Post by vips7L »

rileymat2 wrote: post #73
znpy wrote: post #9

Earlier quoted context omitted.

on a different side: it also tell you (a lot) about specific people. I've seen good/great people call out the nitpicks (in my case it was often mis-spelling, due to not being a native speaker of english) but will approve the PR anyway (implicitly expecting another revision to be sent, trusting the submitter). On the other hand bad/toxic people will drown you with stylistic nitpicks and won't approve (and trust) you t…

In many dynamic languages, depending on scope, spelling is not a nitpick, it has long term mental costs and very well can cause bugs.

Spelling automatically should be highlighted by the ide too. Spelling mistakes means you’re blatantly ignoring warnings from the ide.

Re: PR process killing morale and productivity

#96

Post by bluefirebrand »

arp242 wrote: post #77
bluefirebrand wrote: post #13

Earlier quoted context omitted.

Honestly for me it tells me they need to hook up a code auto formatter into their workflow Forget stylistic nitpicking. Enforce a code quality standard with a linter and formatter and be done with it

It's impossible for any code formatter to be 100%. Where to put a blank line? Where to break a line? How to name a variable/function? etc. etc. Some try (e.g. prettier), and what you end up is frankly just bizarre code that's just ugly. Never mind tons of other small things that often don't really matter. The solution is to just not be too anal about it. It really is a cultural problem. For example a few weeks ago I…

> The solution is to just not be too anal about it. It really is a cultural problem

"Any proposal that requires everyone to just is not a solution, because everyone will not just"

People are anal. You aren't going to get them to stop being anal

A real solution is to have the team agree on a shared style guide, then enforce it with a linter and formatter. If anyone cannot come to an agreement with the rest of the team, or continues to be anal about things that do not appear in the style guide after this, then that person has singled themselves out and the company will need to find a way to deal with the behavior

I agree that good engineers focus more on the actual structure and problems instead of nitpicky things like formatting

That said, code cleanliness and consistency is important too. It makes codebases much easier to maintain and understand if everything is formatter consistently. It's a pretty mid engineer take to think it's not important at all

Re: PR process killing morale and productivity

#97

Post by Jare »

hibikir wrote: post #92
OptionOfT wrote: post #80

Earlier quoted context omitted.

This is why I love things like cargo fmt / go fmt / eslint / etc. No discussions of whether if (foo) return; is valid, or we should do if (foo) { return; }

That's also why I dislike it: The mind that will go apeshit over trivial style nonsense will still provide unhelpful feedback on other topics... but it will become far less obvious that their comments should be downgraded. Formatters also do a poor job if your language is flexible and expressive. Great for go, but if your language is the kind that easily supports internal DSLs, formatters are not even helpful at maki…

> it will become far less obvious that their comments should be downgraded.

I'm curious why you think this would happen... I would imagine that their comments will now have be to about things that matter, and if they are unhelpful they will stand out more.

Re: PR process killing morale and productivity

#98

Post by arp242 »

bluefirebrand wrote: post #96
arp242 wrote: post #77

Earlier quoted context omitted.

It's impossible for any code formatter to be 100%. Where to put a blank line? Where to break a line? How to name a variable/function? etc. etc. Some try (e.g. prettier), and what you end up is frankly just bizarre code that's just ugly. Never mind tons of other small things that often don't really matter. The solution is to just not be too anal about it. It really is a cultural problem. For example a few weeks ago I…

> The solution is to just not be too anal about it. It really is a cultural problem "Any proposal that requires everyone to just is not a solution, because everyone will not just" People are anal. You aren't going to get them to stop being anal A real solution is to have the team agree on a shared style guide, then enforce it with a linter and formatter. If anyone cannot come to an agreement with the rest of the team…

You can't "enforce" all of these things. That was my main point.

And you absolutely can stop people from doing that. Simply accepting dickish asshole toxic behaviour as "well, people are like that shrug" and never telling people off is exactly the problem.

Re: PR process killing morale and productivity

#99

Post by The_Colonel »

bluefirebrand wrote: post #96
arp242 wrote: post #77

Earlier quoted context omitted.

It's impossible for any code formatter to be 100%. Where to put a blank line? Where to break a line? How to name a variable/function? etc. etc. Some try (e.g. prettier), and what you end up is frankly just bizarre code that's just ugly. Never mind tons of other small things that often don't really matter. The solution is to just not be too anal about it. It really is a cultural problem. For example a few weeks ago I…

> The solution is to just not be too anal about it. It really is a cultural problem "Any proposal that requires everyone to just is not a solution, because everyone will not just" People are anal. You aren't going to get them to stop being anal A real solution is to have the team agree on a shared style guide, then enforce it with a linter and formatter. If anyone cannot come to an agreement with the rest of the team…

This:

> "Any proposal that requires everyone to just is not a solution, because everyone will not just"

invalidates this:

> A real solution is to have the team agree on a shared style guide, then enforce it with a linter and formatter.

... since the "team" is everyone. It's basically the same problem.

The other issue is that linters/formatters don't "solve" all formatting/stylistic choices. Most formatters, fortunately, still allow you to choose where you do line breaks for example, since they do matter and shouldn't be arbitrary.

Re: PR process killing morale and productivity

#100

Post by roland35 »

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.

Yes exactly! Reviews should not waste time with format - I would take the hour to set that up in CI and never worry about it again.

Or if that is too much (no shame!), just accept style differences.

Post reply on HN