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

131–140 of 224 posts

Re: PR process killing morale and productivity

#131

Post by chipdart »

bdangubic wrote: post #129
chipdart wrote: post #126

Earlier quoted context omitted.

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

Services like GitHub and GitLab support preventing changes that overwrite a repo's history, in addition to tracking history even when you force update a branch.

Re: PR process killing morale and productivity

#132

Post by chipdart »

paiute wrote: post #36
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.

I like this idea, it has the bonus of revealing how much the reviewer actually cares. I bet half the things they wouldn’t take the time to fix themselves.

> I like this idea, it has the bonus of revealing how much the reviewer actually cares.

And now you contributed to turn your team culture into shit, with a toxic mix of PR creators purposely shitting on the team's style guide and PR reviewers who have to constantly post follow-up commits to your work because you can't even put together and acceptable PR or clean up after yourself.

What if anyone who posts a PR addresses feedback anyone posts on their work and addresses them until you get the necessary and sufficient approvals?

Re: PR process killing morale and productivity

#133

Post by chipdart »

roland35 wrote: post #100
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.

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.

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

What if you post a PR and forgot to run a linter or configure your editor? Should that not justify a comment over style violations?

The problem will only go away if everyone is on the same page.

Re: PR process killing morale and productivity

#134

Post by philwelch »

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…

Can you give an example of what you mean by a language supporting “internal DSL’s”?

Re: PR process killing morale and productivity

#135

Post by chipdart »

collingreen wrote: post #62
eddd-ddde wrote: post #44

Earlier quoted context omitted.

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.

This highlights something very important to me that sometimes gets lost when thinking about the tooling and that is the human element and the importance of communication, existing relationships, and rapport. The PR feedback I give and receive is extremely influenced by the relationship I have with the person - for some PRs any nit pick feels frustrating but I have other coworkers that can slam my code and even pepper…

> The PR feedback I give and receive is extremely influenced by the relationship I have with the person - for some PRs any nit pick feels frustrating (...)

Posting PR comments doesn't prevent you from accepting the PR. You can post a torrent of nit comments and still approve it so that minor issues don't turn into blockers.

Re: PR process killing morale and productivity

#136

Post by philwelch »

chipdart wrote: post #133
roland35 wrote: post #100

Earlier quoted context omitted.

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.

> Reviews should not waste time with format - I would take the hour to set that up in CI and never worry about it again. What if you post a PR and forgot to run a linter or configure your editor? Should that not justify a comment over style violations? The problem will only go away if everyone is on the same page.

CI should reject the feature branch if the linter fails. Never waste an engineer’s time doing work a program can do.

It’s also often handy to configure linters and autoformatters as precommit hooks.

Re: PR process killing morale and productivity

#137

Post by chipdart »

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

> Enforce a code quality standard with a linter and formatter and be done with it

Adopting a linter does not eliminate style issues. It just eliminates the number of possible comments a PR can get over style issues by replacing all detailed feedback with just one comment with a very clear actionable request that's trivial to satisfy: "It seems X doesn't look right. Could you please run the linter?"

Re: PR process killing morale and productivity

#138

Post by pavel_lishin »

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

I don't particularly care which one you choose - although I admit to having a preference - but I want that style consistently applied throughout the project, so I'm not stumbling over stylistic differences as I'm scrolling through code trying to investigate something.

And keep in mind that this particular example, that some people can read clearly, is just five lines. When you're looking at a 3k line file, one of the dozen you're digging through during an outage, that shit matters.

Re: PR process killing morale and productivity

#139

Post by pavel_lishin »

lapcat wrote: post #32
zie1ony wrote: post #10

Recently I allowed everyone in my team to push to develop without PRs and even without doing feature branches. We review the code all together (max 2 ppl) just before the release and that's it. But great CI pipeline is must for this. It will get even better soon when AI will be able to slightly refactor the code.

> We review the code all together (max 2 ppl) just before the release and that's it. Is there a reason why you don't review the code immediately? You could also do that without PRs.

Yeah, I've heard arguments for this, and it always feels weird to me. Why go so long without review? That's just giving developers - junior and senior - opportunity to dig themselves into a hole that they don't notice until someone points out, "hey, your last five commits are buggy, but that matters less than the fact that they're implementing the wrong thing."

Re: PR process killing morale and productivity

#140

Post by climb_stealth »

meowfly wrote: post #124
mihaaly wrote: post #107

Earlier quoted context omitted.

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.

Bugs. Subtle horrible bugs that can take forever to find. Not having the braces makes it so easy to accidentally break in later changes or refactoring. Sure on its own it's super obvious, but next to other changes or refactoring it is really easy to miss that suddenly logic falls out of the condition or doesn't make sense anymore.

I don't remember the details but I've had some typo in a one-line-if-condition once and it took me days to find it. Might have been an accidental semicolon in C. A linter enforced the braces and line-breaks and made it obvious.

Anyways, running a linter also helps because all anger or frustration can be directed towards the machine.

Post reply on HN