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

151–160 of 224 posts

Re: PR process killing morale and productivity

#151

Post by sundbry »

Kinrany wrote: post #30
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

This, you should make soft and hard limits on PR sizes, providing exceptions only for automated changes or rare situations such as structural changes.

Re: PR process killing morale and productivity

#152

Post by sundbry »

loa_in_ wrote: post #109
lijok wrote: post #63

Earlier quoted context omitted.

Speaks volumes to the maturity of our work that a 3000 LOC PR is seen not only as acceptable, but expected. Raising a planning change equivalent to a 3000 LOC PR in a civil engineering firm would get your assignment swiftly handed over to someone more competent.

LOC is a bad measure of literally anything. It's not related to productivity, efficiency, bug count, readability, etc. What it's most related to is the choice of language and libraries. For me, shunning big contributions just for being big just stifles my drive to contribute. If the solution does what it's supposed to, is valid, efficient and there's no obvious unnecessary duplication of codethen who cares if it's bi…

It is actually directly related to the time and complexity of code review. Just because LOC is an imperfect metric for productivity etc., does not mean it's a poor metric for risk, complexity, poor sequencing of changes, or other attributes.

Re: PR process killing morale and productivity

#153

Post by avidiax »

chipdart wrote: post #125
hibikir wrote: post #92

Earlier quoted context omitted.

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

I have lots of experience working with software.

Linting and style guides are not of "critical importance". No business objective will go unachieved because some checkins use tabs and others use spaces.

Whatever problem style issues might cause can be resolved before lunch by running a formatter and committing the result.

Re: PR process killing morale and productivity

#155

Post by mshekow »

I found two ideas / techniques helpful in this context:

1) Conventional comments (https://conventionalcomments.org/) as an (agreed-upon) language to be used in PR comments

2) Ship / Show / Ask (https://martinfowler.com/articles/ship-show-ask.html), where "Show" and "Ship" are non-blocking PRs (or even directly committing to trunk, if you use trunk-based development), since not every(!) PR needs reviewing and/or should block the PR creator

Re: PR process killing morale and productivity

#156

Post by Jare »

philwelch wrote: post #134
hibikir wrote: post #92

Earlier quoted context omitted.

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”?

Scala would be my immediate thought. Probably also C/C++ with macros.

Re: PR process killing morale and productivity

#157

Post by greatgib »

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.

In lots of case linters might be harmful also. It is one thing to have a style guide people follow in most cases, it is another one to rigidly force everywhere the style that the linter enforce.

Especially because most linters will impose that everyone on the team will use them.

For example, if we take Python, there is black that is bat shit and makes your code less readable but adopted by so many teams as cargo cult because doing that signal that you are a cool, hype, best practice following team...

In the end with "black", people are forced to abide by the style of the random guy that created it, that is not exactly following the PEP8 or the zen of python, and is just itself created in cargo cult inspired by go and rust, and Python creator even advised against using it for readability except in special cases like very very big teams.

Re: PR process killing morale and productivity

#158

Post by majkinetor »

pavel_lishin wrote: post #138
mihaaly wrote: post #107

Earlier quoted context omitted.

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, on…

Its not only that. Style may signify more than just a cosmetics, it can lead to vastly different code understanding.

Re: PR process killing morale and productivity

#159

Post by emptiestplace »

avidiax wrote: post #153
chipdart wrote: post #125

Earlier quoted context omitted.

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

I have lots of experience working with software. Linting and style guides are not of "critical importance". No business objective will go unachieved because some checkins use tabs and others use spaces. Whatever problem style issues might cause can be resolved before lunch by running a formatter and committing the result.

> resolved before lunch

blame.ignoreRevsFile

Re: PR process killing morale and productivity

#160

Post by em-bee »

chipdart wrote: post #142
bluefirebrand wrote: post #96

Earlier quoted context omitted.

> The solution is to just not be too anal about it. It really is a cultural problem "Any proposal that requires everyone to just is not a solution, because everyone will not just" People are anal. You aren't going to get them to stop being anal A real solution is to have the team agree on a shared style guide, then enforce it with a linter and formatter. If anyone cannot come to an agreement with the rest of the team…

> I agree that good engineers focus more on the actual structure and problems instead of nitpicky things like formatting I don't think you understand that formatting is of critical importance. Sure, your code won't break if you add a space at the right or at the left of a symbol. But your code will be reformatted the next time someone like you works on that file and takes the same naive approach to that code that you…

style fixes should always be separate commits. if they fix a PR, they should be part of that PR or an independent PR, but ideally not included in the next code changing PR
Post reply on HN