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

201–210 of 224 posts

Re: PR process killing morale and productivity

#201

Post by mckn1ght »

Joker_vD wrote: post #27
barbazoo wrote: post #22

Earlier quoted context omitted.

Even a simple (and very appropriate for a junior/newbie) change like you suggested could invite dozens of comments if the engineers don't know what they're doing.

"Dozens" means "at least 24", so that'd been slightly more than 2 comments per line in my example. Which is an insane amount of comments by any standard imaginable: because long before you reach 1-1 comments-to-code ratio, you'd stop, and write a single "no, all of this is a wholly incorrect approach, complete re-do is needed" comment instead.

You’re right, but some people out there will flex and grandstand in their reviews if they operate adversarially instead of collaboratively, which is what your suggestion would encourage.

Re: PR process killing morale and productivity

#202

Post by swisniewski »

I usually adopt a policy with my teams:

You are not allowed to complain about style in code reviews.

If it’s important enough for your to comment about, it’s important enough for you to add a linter to the build and block the build if the style isn’t met.

If it’s not worth enough of your time to add a linter, it’s certainly not worth your peer’s time to deal with comments about style.

Re: PR process killing morale and productivity

#203

Post by plasticchris »

Aeolun wrote: post #189
plasticchris wrote: post #185

Earlier quoted context omitted.

That’s why you hook the linter up so it runs on every commit / push. No need to ask, it always runs. And no need to quibble over style. Don’t like it? Change the linter.

I very much dislike any process modifying my commit after I submit it. Rebasing becomes hell.

run it in a hook that runs before the commit is made

Re: PR process killing morale and productivity

#204

Post by greatgib »

jessekv wrote: post #184
greatgib wrote: post #157

Earlier quoted context omitted.

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…

> not exactly following the [...] zen of python Blacks's stance that "there should be one obvious way to format things" seems consistent with the zen, even if you disagree with the actual rules.

For me it is not respecting the zen of python by pushing you to bad formatting sometimes just because the tool requires you to.

And just for reminder, the following is one important point at the beginning of the pep8:

    Foolish Consistency is the Hobgoblin of Little Minds

Re: PR process killing morale and productivity

#205

Post by nemetroid »

chipdart wrote: post #167
nemetroid wrote: post #165

Earlier quoted context omitted.

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?

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

Just because you don't value or acknowledge them doesn't mean they don't exist.

Re: PR process killing morale and productivity

#206

Post by climb_stealth »

Timwi wrote: post #166
climb_stealth wrote: post #140

Earlier quoted context omitted.

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.

I was thinking of a case like this:

    if (foo)
        return;
Perfectly valid and people do it. But then later on someone might put a debug statement above the return to check something. Or add some other logic.

    if (foo)
        doSomething();
        return;
And suddenly the logic is broken and the return falls out of the if-condition. It looks fairly contrived with an example like this but easy to do when code is a bit more complicated or people are tired or rushed. It's a good habit to always enforce braces for if-conditions as it defeats that whole category of mistakes. Fairly innocent but can be so hard to find because of that.

A linter would enforce having braces and solve the issue. And for codebases without a linter it is a good habit to just go with the braces.

Re: PR process killing morale and productivity

#207

Post by scotty79 »

Code reviews are weird. I joined a very small team as a new developer. I got a task, solved it but at code review the senior developer told me to solve it in another specific way. I coded it and it was fine. The situation repeated for few next tasks. It was a bit frustrating because I felt like doing the job twice while "pre-review" of my tasks by the ultimate code reviewer before I started doing them could save both my time and his. But it just wasn't part of the process.

Does such thing have a name? A quick check up with a reviewer about whether my plan for implementing the task is align with their preferences?

Re: PR process killing morale and productivity

#208

Post by philwelch »

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…

> Your CI pipeline is broken if it refuses to run because of style issues.

The whole point of CI is to automatically verify the code. Linters are a method of doing that, the same as tests.

> Imagine going into a meeting with a senior manager and explain that you cannot release a hot fix because your pipeline is broken due to the last commit having 5 spaces instead of 4.

Sounds like an easy fix to me. And if your CI is set up properly, the misformatted branch wouldn’t have been merged to master in the first place.

Re: PR process killing morale and productivity

#209

Post by philwelch »

chipdart wrote: post #167
nemetroid wrote: post #165

Earlier quoted context omitted.

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?

Who’s blocking any pipelines here? If you’re running a PR process at all, these changes are being pushed to a feature branch and reviewed before getting merged to master. If you have a feature branch that’s failing CI, that doesn’t block me from merging my feature branch once my PR is approved.

As I specifically said, the CI should fail on the feature branch. If you’re only running CI on master, you’re going to run into the exact same problem if your unit tests fail. The way to avoid that problem is to run the unit tests on your feature branch, and if you’re doing that you might as well run the linters too.

Re: PR process killing morale and productivity

#210

Post by chipdart »

philwelch wrote: post #208
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…

> Your CI pipeline is broken if it refuses to run because of style issues. The whole point of CI is to automatically verify the code. Linters are a method of doing that, the same as tests. > Imagine going into a meeting with a senior manager and explain that you cannot release a hot fix because your pipeline is broken due to the last commit having 5 spaces instead of 4. Sounds like an easy fix to me. And if your CI i…

> The whole point of CI is to automatically verify the code.

Against errors and regressions. Meaning, stuff that breaks your code and affects the service you provide to users.

Style issues ain't that. Come on.

Post reply on HN