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

71–80 of 224 posts

Re: PR process killing morale and productivity

#71

Post by powersnail »

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 that it's not a decided thing, and you can't prosecute me with a law that hasn't passed yet. Once it's actually settled, I'll be happy to go back and refactor it, but it is illogical to make it a blocker in the present.

It also sounds like something is wrong with the on-boarding process. The first PR should not be of such a nature that would invite loads of comments. It should be something that is functionally simple, and already has an agreed-upon design.

Re: PR process killing morale and productivity

#72

Post by plorkyeran »

loeg wrote: post #65
praptak wrote: post #59

Earlier quoted context omitted.

Linux C style guide has this summarized pretty well: "The maximum length of a function is inversely proportional to the complexity and indentation level of that function. So, if you have a conceptually simple function that is just one long (but simple) case-statement, where you have to do lots of small things for a lot of different cases, it’s OK to have a longer function. However, if you have a complex function, and…

This is essentially a rough measure of cyclomatic complexity.

Unfortunately I think that cyclomatic complexity is a rough measure of this. It’s an attempt at an objective measurement, but I’ve never found it to be a particularly good one.

Re: PR process killing morale and productivity

#73

Post by rileymat2 »

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…

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

Re: PR process killing morale and productivity

#74

Post by steveBK123 »

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.

Re: PR process killing morale and productivity

#75

Post by marcosdumay »

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

This. You either don't care about style or push the style automatically without any interaction. Every other option is bad to some extent.

But only use a linter if you will add your rules over the empty set. If you get a pre-built set and are expected to remove the ones you don't need, you are making a toxic environment.

Re: PR process killing morale and productivity

#76

Post by SketchySeaBeast »

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.

Yeah, if you care about it enough to stop a PR because of something and that something can be automated, automate it.

Re: PR process killing morale and productivity

#77

Post by arp242 »

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

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 reviewed a PR from a new team member; there were some seriously structural problems with his approach, so I commented on that and ignored all the small stuff for now. Another programmer on my team also reviewed at the same time, and only commented on the small stuff that, IMHO, don't really matter, and didn't look at the general approach at all (which really was just all wrong, and also quite obviously wrong).

Not to be too arrogant about it, but I feel this sort of stuff is what distinguishes a "good engineer" from a "mid engineer".

Re: PR process killing morale and productivity

#78

Post by skydhash »

BurningFrog wrote: post #53
gboss wrote: post #12

Whenever I encounter a pull request that I find many issues with, I ask to meet with the engineer and review it one on one. More than half of the workplace problems engineers have is due to their introverted nature and their refusal to get on a teams/zoom call to explain their issues and get resolution. Comments are a really poor mechanism for teaching programming best practices.

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

Re: PR process killing morale and productivity

#79

Post by arp242 »

praptak wrote: post #59
jchw wrote: post #20

Personally I have a strong distaste for projects that try to use some metric for how long a function should be, e.g. line count or cyclomatic complexity. I'm not sure if this one is better automated, human judgement for what makes sense seems better to me. Sometimes the cleanest and highest performance way to write some code is going to be basically one relatively large function. If there's a function complexity/size…

Linux C style guide has this summarized pretty well: "The maximum length of a function is inversely proportional to the complexity and indentation level of that function. So, if you have a conceptually simple function that is just one long (but simple) case-statement, where you have to do lots of small things for a lot of different cases, it’s OK to have a longer function. However, if you have a complex function, and…

What really matters is "can I understand what this function roughly does at a glance?" In general, "length of function" is a reasonably-ish proxy for it, but as your quote says, sometimes longer is okay, and sometimes there are also other factors than just "length of function".

Either way, the question you should be asking during review is "can I understand this function?" and not "how long is this function?"

Re: PR process killing morale and productivity

#80

Post by OptionOfT »

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;
    }
Post reply on HN