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

121–130 of 224 posts

Re: PR process killing morale and productivity

#121

Post by mixmastamyk »

yjftsjthsd-h wrote: post #45
PhilipRoman wrote: post #29

Earlier quoted context omitted.

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.

Not just stylistic choices - I absolutely love the way ex. gitlab lets you do a suggested change in a comment, precisely because it lets me do all the work and the MR owner just has to click to accept if they agree. That lowers the bar for what I'm willing to bring up in the first place (though I personally still don't like to comment on matters of style or taste) while making it less work for everyone, which makes i…

Is that new? I don’t remember that feature, working with gitlab a few months back.

Re: PR process killing morale and productivity

#122

Post by chipdart »

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.

> Most of it was stylistic nitpicking.

One man's "stylistic nitpicking" is another man's violation of the company's official coding guidelines.

Does the company adopted a linter? If so,why has the new hire not applied it before posting the PR?

Imagine a new hire throwing a hissy fit because even though the whole company writes code in PascalCase he feels that snake_case is better.

Imagine a new hire decided that tabs are better than spaces, and proceeded to reformat 30% of a source file leaving behind wonky indentation.

Shall we tolerate code that has both just so that random internet people in a random online forum can rail against PRs?

Or shall we post a comment in a PR pointing why the code should be reformatted?

Re: PR process killing morale and productivity

#123

Post by chipdart »

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; }

> This is why I love things like cargo fmt / go fmt / eslint / etc.

I agree. Once I had the displeasure of working with a junior dev who was very prolific in posting comments on style and if a space should be at the left or at the right of a symbol. It took me a few days of dealing with that noise to onboard a linter.

Even so the junior dev felt entitled to manifest how high their standards were by posting a torrent of comments in the PR that onboarded the team's official linter config.

But once the PR was approved and merged, surprise surprise: the junior dev's PR comment metrics dropped from dozens per PR to zero. The style guide didn't even had to be enforced.

The last I've heard about the junior dev was throwing a tantrum when one of their PRs received a comment from another team member asking to run the linter because it failed to adhere to the team's official style guide. Apparently the high standards and this attention to detail only went one way.

Re: PR process killing morale and productivity

#124

Post by meowfly »

mihaaly wrote: post #107
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; }

Who cares?!

I agree. I can read both of them clearly. I'm not sure what value we are maximizing for in this example. It's definitely not readability.

Re: PR process killing morale and productivity

#125

Post by chipdart »

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…

> over trivial style nonsense

What's your definition of "trivial style nonsense"? Is it "something I personally don't understand or care"?

Things like tabs vs spaces is important, and so is how many spaces an indentation level should take. This affects how editors reformat code, how other people's editors present the code, and even how many lines a commit has and is blamed for a change.

That's why linters are of critical importance to a team, and so is adopting a shared standard and editor config.

The only people who don't understand the importance of a style guide and how to make it a non-issue by enforcing tools to enforce it are those with little to no experience working with software.

Re: PR process killing morale and productivity

#126

Post by chipdart »

mixmastamyk wrote: post #121
yjftsjthsd-h wrote: post #45

Earlier quoted context omitted.

Not just stylistic choices - I absolutely love the way ex. gitlab lets you do a suggested change in a comment, precisely because it lets me do all the work and the MR owner just has to click to accept if they agree. That lowers the bar for what I'm willing to bring up in the first place (though I personally still don't like to comment on matters of style or taste) while making it less work for everyone, which makes i…

Is that new? I don’t remember that feature, working with gitlab a few months back.

> Is that new?

It's neither new nor novel. Even GitHub allows anyone to post a commit on a feature branch, regardless of who created it.

It's not a GitLab or GitHub feature. It's a Git feature: the ability to post a commit to a branch.

Re: PR process killing morale and productivity

#127

Post by chipdart »

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 to handle stylistic choices (if at all) is to have the reviewer just adjust it themselves.

How does that inform the PR author that they failed to pay attention to a relevant detail, and more importantly contribute to not make it again?

If there is no feedback, problems will persist.

Nit comments add noise and failures to comply with a style guide is always a distraction on a PR, but that does not mean style guide violations should be ignored. It means that your team must work their way into making them a non-issue.

This means establishing an objective and usable set of rules and guidelines, and enforce them programmatically with a linter, not turning a blind eye to style violations.

Re: PR process killing morale and productivity

#128

Post by chipdart »

grecy wrote: post #89
PhilipRoman wrote: post #29

Earlier quoted context omitted.

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.

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

That would be something only a terribly incompetent manager would ever do.

A manager who is not terribly incompetent would either not take sides until explicitly asked to weigh in, and if and only if he was dragged into the discussion then the manager's only acceptable input would be to eliminate the problem for good by adopting a linter.

Everything else is a mistake and a complete waste of time.

Re: PR process killing morale and productivity

#129

Post by bdangubic »

chipdart wrote: post #126
mixmastamyk wrote: post #121

Earlier quoted context omitted.

Is that new? I don’t remember that feature, working with gitlab a few months back.

> Is that new? It's neither new nor novel. Even GitHub allows anyone to post a commit on a feature branch, regardless of who created it. It's not a GitLab or GitHub feature. It's a Git feature: the ability to post a commit to a branch.

create alias dont-fuck-with-my-branch

git reset --hard HEAD~1

git push -f

:)

Re: PR process killing morale and productivity

#130

Post by chipdart »

johannes1234321 wrote: post #82
herpdyderp wrote: post #39

Earlier quoted context omitted.

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 member…

> There is a lot of stuff a linter can't do (...)

Whatever a linter can't do, it's not worth doing.

> Naming being a big one.

You're confusing personal opinions over how to name things with concrete style issues.

Post reply on HN