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

41–50 of 224 posts

Re: PR process killing morale and productivity

#41

Post by agentultra »

Yes!

I’d also add that it’s helpful to have a review guideline.

When I get to the PR phase, I’ve already done the design work, considered the trade-offs, and am asking folks to check that the code is mergeable per our guidelines.

A pet peeve of mine are reviewers with a bone to pick who will leaving blocking comments to redo the work in the way they would have approached it.

A guideline helps here because you can, along with your team, manage expectations for the process.

Some folks like to throw up code for critiques. They use PRs to get feedback on the design/approach itself. That can be useful!

Going in to manage expectations helps smooth things along so that you’re giving the constructive feedback the author is looking for.

Re: PR process killing morale and productivity

#42

Post by edflsafoiewq »

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

Style also involves things like "renaming variables", to take the article's example, which can't be automated away.

Re: PR process killing morale and productivity

#43

Post by herpdyderp »

eddd-ddde wrote: post #37
BurningFrog wrote: post #11

Another angle is that a PR with over 300 comments is probably way too big. Many small focused PRs is IMAO much better than big PRs with weeks or months of work in them. But that requires fast and friendly PR reviews!

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

Re: PR process killing morale and productivity

#44

Post by eddd-ddde »

js8 wrote: post #26
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.

For me it's the opposite. If I'm making a PR, please point out ALL of the nits you can even think about. My next PR is bound to have at most 25% of that since by now I get more about what the team values and considers important.

Re: PR process killing morale and productivity

#45

Post by yjftsjthsd-h »

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.

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 it much easier to give useful suggestions:)

Re: PR process killing morale and productivity

#46

Post by stared »

One core issue is that PRs are often too large. I don't believe there are 300 comments on a small PR unless the team is dysfunctional.

Aim to make them as atomic as possible. If it involves adding a significant feature that requires substantial changes to other modules, create a separate branch for it. Within this branch, break the work down into a series of smaller PRs.

When PRs are too large, the process usually suffers from insufficient reviews, excessive nitpicking, or (most often) a combination of both.

Re: PR process killing morale and productivity

#47

Post by bsnnkv »

I can't imagine going back to working on codebases in languages that don't come with strongly opinionated defaults.

I'm very lucky to spend the majority of my dayjob hours working on Rust code, so everyone just runs cargo fmt && cargo clippy, and this in enforced in CI. You can't even publish a PR until those basic bars have been met.

I can't imagine the absolute insanity of working on JS projects where there are more ways to do things than there are people on a team, and where getting those people with strongly held opinions about things that ultimately just don't matter to agree on style and convention is like pulling teeth.

Re: PR process killing morale and productivity

#48

Post by yjftsjthsd-h »

dec0dedab0de wrote: post #21
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…

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

Depending on your company culture, you can also just explicitly write something like "This is not a blocker, but I would suggest...". Of course if your culture uses explicit "press the button to unblock" then that's probably redundant.

Re: PR process killing morale and productivity

#49

Post by yjftsjthsd-h »

treespace8 wrote: post #4

GIT seams to be optimized for network of trust. With one person at the top approving what gets merged into the release. This person of course does not do all of the verification, other then broad strokes of what the change does, and who wrote it, reviewed it and tested it. I feel like companies do not want a large tree like structure for their development teams. Without a network of trust it can become mob rule, whic…

That makes sense; https://en.wikipedia.org/wiki/Conway%27s_law applies and git was created by Linus Torvalds for Linux, which works exactly like that.

Re: PR process killing morale and productivity

#50

Post by to11mtm »

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…

It's tough with spelling sometimes.

In my current role I review a lot of PRs, they tend to be large due to the waterfall way things work... If I don't ask to fix the spelling now, It might not happen for a year or two. That said if it can be fixed in separate PR before release, that's fine.

Really, it's best to have some terms or explicitness.

For example, with my teams 'Nitpick' means I'm just being nitpicky, Doesn't have to change unless it's on the edge (and I'm explicit as to why it should change, i.e. I know the next thing will need the change anyway). "Consider" means It doesn't have to happen, but here's some food for thought. "PLZ FIX" is fairly self explanatory.

Also, making sure management (especially for contract houses) knows that PRs are a 'judgement free zone' and should not be held against people for perf reviews etc; that should be collected by other peer feedback channels instead.

Post reply on HN