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

21–30 of 224 posts

Re: PR process killing morale and productivity

#21

Post by dec0dedab0de »

znpy wrote: post #9
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.

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…

* 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).*

I always thought this was the best way.

I wish these systems had a way to assign severity to comments, and urgency to the commit.

If you have a jr developer it is your job to give them stylistic feedback, the problem comes from mixing it in with security holes or sneaky bugs. And when the process doesn’t identify when we need to ship it yesterday, vs in the next few months.

Re: PR process killing morale and productivity

#22

Post by barbazoo »

Joker_vD wrote: post #8

...just how big was that "first ever contribution" pull request that it gathered 300 comments? My first commit in the current company was a 2-liner addition to an existing function plus another 20 lines for a test that verified that indeed, the change does affect the outcome.

Even a simple (and very appropriate for a junior/newbie) change like you suggested could invite dozens of comments if the engineers don't know what they're doing.

Re: PR process killing morale and productivity

#23

Post by donatj »

My team has a bike shedding sort of problem where a 100 loc PR will sometimes get scrutinized to hell, but a 3,000 loc PR will get LGTM'd by enough of the team to be merged before anyone that actually cares gets a chance to look at it. I would say the second half of that is the much bigger problem.

People know who to ask to get a quick lgtm.

I don't know what to do about it. I can't make people actually review. I've had unrealistic dreams like holding people responsible for things they approved that had bugs, but anything like that would be supremely unpopular. I could do something like requiring review from the team members that actually review, but they already feel overwhelmed by being the only ones that actually review.

We tried setting soft limits on the size of PRs, but that comes with a lot of PR that are hard to review because the work is poorly divided and doesn't make sense in isolation.

Re: PR process killing morale and productivity

#24

Post by InvaderFizz »

bluefirebrand wrote: post #13
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.

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

I agree. Even within my own org there is some nitpicking, but it's almost always about style consistency for our shared codebase, which is valid.

If your PR doesn't pass lint checks, it doesn't get merged. And the only reason it would fail the lint checks is if your pre-commit hooks didn't fire.

There is no argument of 2,4,8 space vs tabs, because the code you commit is run through the linter.

Write however you want for the things that don't matter, the formatter always wins.

Re: PR process killing morale and productivity

#26

Post by js8 »

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.

I think a good general advice when training somebody is - focus them only on the biggest flaw at the time. That's how I would proceed with a "bad" PR.

I would even accepts smaller issues (style) when there is a bigger issue to fix. People might eventually outgrow it.

Re: PR process killing morale and productivity

#27

Post by Joker_vD »

barbazoo wrote: post #22
Joker_vD wrote: post #8

...just how big was that "first ever contribution" pull request that it gathered 300 comments? My first commit in the current company was a 2-liner addition to an existing function plus another 20 lines for a test that verified that indeed, the change does affect the outcome.

Even a simple (and very appropriate for a junior/newbie) change like you suggested could invite dozens of comments if the engineers don't know what they're doing.

"Dozens" means "at least 24", so that'd been slightly more than 2 comments per line in my example. Which is an insane amount of comments by any standard imaginable: because long before you reach 1-1 comments-to-code ratio, you'd stop, and write a single "no, all of this is a wholly incorrect approach, complete re-do is needed" comment instead.

Re: PR process killing morale and productivity

#28

Post by simmschi »

donatj wrote: post #23

My team has a bike shedding sort of problem where a 100 loc PR will sometimes get scrutinized to hell, but a 3,000 loc PR will get LGTM'd by enough of the team to be merged before anyone that actually cares gets a chance to look at it. I would say the second half of that is the much bigger problem. People know who to ask to get a quick lgtm. I don't know what to do about it. I can't make people actually review. I've…

Have you tried the code owners feature (assuming you're on Github).

IMO a good approach is to have the actual code owners (i.e. the team responsible for a specific service or library) review the PR. If they think a shallow LGTM review of 3k LOC is enough, they can also deal with the bugs :-)

If you don't have specific ownership in your code base I'd start there.

Re: PR process killing morale and productivity

#29

Post by PhilipRoman »

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.

Re: PR process killing morale and productivity

#30

Post by Kinrany »

donatj wrote: post #23

My team has a bike shedding sort of problem where a 100 loc PR will sometimes get scrutinized to hell, but a 3,000 loc PR will get LGTM'd by enough of the team to be merged before anyone that actually cares gets a chance to look at it. I would say the second half of that is the much bigger problem. People know who to ask to get a quick lgtm. I don't know what to do about it. I can't make people actually review. I've…

Anything you do to make people fix their 3000 loc PRs will be unpopular with those people
Post reply on HN