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

161–170 of 224 posts

Re: PR process killing morale and productivity

#161

Post by znpy »

rileymat2 wrote: post #73
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…

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

I should have been less ambiguous: i meant spelling of words on the English language, in comments.

Good pr systems will perform a build of the code and run tests, so if the mis-spelling is in the code the build will fail anyway.

Re: PR process killing morale and productivity

#162

Post by znpy »

vips7L wrote: post #95
rileymat2 wrote: post #73

Earlier quoted context omitted.

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

Spelling automatically should be highlighted by the ide too. Spelling mistakes means you’re blatantly ignoring warnings from the ide.

Please see my other comment. I meant misspelling of English words in the comments. Misspelled code will not compile and/or pass the tests so the pr won’t be accepted anyway.

Re: PR process killing morale and productivity

#163

Post by chipdart »

em-bee wrote: post #160
chipdart wrote: post #142

Earlier quoted context omitted.

> 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

> style fixes should always be separate commits.

Not really. If you're already changing the code and running linters afterwards introduces changes over your change, this means you are the one introducing the problems. Separate commits just add noise.

Your comment is like saying that bug fixes should be separate commits when arguing about how not to add bugs to begin with.

Re: PR process killing morale and productivity

#164

Post by chipdart »

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.

> Linting and style guides are not of "critical importance".

This is simply false, as attested by the huge volume of comments in this thread by those with actual professional experience working on real-world software projects.

You're also oblivious to the problem domain, because otherwise you'd understand that the critical problems are not whether a space should be at the left or at the right of a symbol, but all the churn that is required to manually address style problems in PRs.

Try to think about the problem. You post a PR that screws up all formatting. It takes time for a team member to review a PR. Once you start to get reviews,you notice comments pointing out failures in complying with a specific style. Whether you go the passive-aggressive path of waiting for any other team member to review your code or you do the right thing and fix the problems you introduced, that requires another round of PR reviews. The time that you take with each iteration is the time your work is delayed to be merged. Now think about how many hours per month you waste just because you can't manage to format your code properly.

Re: PR process killing morale and productivity

#165

Post by nemetroid »

chipdart wrote: post #143
philwelch wrote: post #136

Earlier quoted context omitted.

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.

> CI should reject the feature branch if the linter fails. Your CI pipeline is broken if it refuses to run because of style issues. Linting is either applied as a pre commit hook or manually by the developer. Anything else is a mistake you are making without any concrete tradeoff. The same goes for other mistakes such as handling warnings as errors. Imagine going into a meeting with a senior manager and explain that…

Just make sure that the pipeline is fast, or allow an override, and there is no issue.

Re: PR process killing morale and productivity

#166

Post by Timwi »

climb_stealth wrote: post #140
meowfly wrote: post #124

Earlier quoted context omitted.

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…

> Not having the braces makes it so easy to accidentally break in later changes or refactoring.

Did you mean “not having a linter”? Because once you have a linter you no longer need the braces because the autoformatting will always fix the indentation.

Re: PR process killing morale and productivity

#167

Post by chipdart »

nemetroid wrote: post #165
chipdart wrote: post #143

Earlier quoted context omitted.

> CI should reject the feature branch if the linter fails. Your CI pipeline is broken if it refuses to run because of style issues. Linting is either applied as a pre commit hook or manually by the developer. Anything else is a mistake you are making without any concrete tradeoff. The same goes for other mistakes such as handling warnings as errors. Imagine going into a meeting with a senior manager and explain that…

Just make sure that the pipeline is fast, or allow an override, and there is no issue.

Requiring manual intervention to handle a blocked pipeline over a non-issue defeats the whole purpose of continuous integration, not to mention that you are suggesting adding twice the complexity as an alternative to not adding any complexity at all.

And should I stress again that there is absolutely zero positive tradeoffs?

Re: PR process killing morale and productivity

#168

Post by Larrikin »

chipdart wrote: post #122
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.

> Most of it was stylistic nitpicking. One man's "stylistic nitpicking" is another man's violation of the company's official coding guidelines. Does the company adopted a linter? If so,why has the new hire not applied it before posting the PR? Imagine a new hire throwing a hissy fit because even though the whole company writes code in PascalCase he feels that snake_case is better. Imagine a new hire decided that tabs…

Using snake_case everywhere when you should be using camelCase should have been one or two comments at most. It's toxic to go through every use and comment

Re: PR process killing morale and productivity

#169

Post by em-bee »

chipdart wrote: post #163
em-bee wrote: post #160

Earlier quoted context omitted.

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

> style fixes should always be separate commits. Not really. If you're already changing the code and running linters afterwards introduces changes over your change, this means you are the one introducing the problems. Separate commits just add noise. Your comment is like saying that bug fixes should be separate commits when arguing about how not to add bugs to begin with.

ok, yes, you should not introduce style problems to begin with. but if the problem is already there from an older PR, it should be fixed separately.

Re: PR process killing morale and productivity

#170

Post by zie1ony »

pavel_lishin wrote: post #139
lapcat wrote: post #32

Earlier quoted context omitted.

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

Good point, but my team is small and has only top engineers with both soft and hard skills. Many times you can do much more with team of 3 than team of 10 ppl.
Post reply on HN