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

141–150 of 224 posts

Re: PR process killing morale and productivity

#141

Post by chipdart »

arp242 wrote: post #77
bluefirebrand wrote: post #13

Earlier quoted context omitted.

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…

> It's impossible for any code formatter to be 100%. Where to put a blank line? Where to break a line?

Not true. Those are objectively covered by any linter.

> How to name a variable/function?

Unless the issue is things like snake_case vs PascalCase, that's not the job of a linter. That's exactly what PR comments are about.

> Some try (e.g. prettier), and what you end up is frankly just bizarre code that's just ugly.

Not true. Without Prettier you always get bizarre code that's just ugly. You don't notice because you paid no attention to the code you wrote, it followed your personal subjective opinion you happened to have at that moment, and you didn't felt strongly enough to verify it.

Without a linter, others would certainly point out the problems they saw in your PR as that would certainly not comply with their personal subjective opinion they might hold at that moment and not before or after.

The importance of Prettier is that it objectively enforces a set of rules. If it's ugly it's because you configured it to be ugly, but it's less of a problem because it will be objectively, systematically and reproducibly ugly.

Re: PR process killing morale and productivity

#142

Post by chipdart »

bluefirebrand wrote: post #96
arp242 wrote: post #77

Earlier quoted context omitted.

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…

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

That leads to PRs having a larger code footprint for no reason other than fixing the previous PR's failure to comply with a style guide.

This means tools like Git blame start to flag parts of the code as having changed recently just because you failed to pay attention to the style guide.

Now that regression that was introduced by an unrelated commit becomes slightly harder to track because it's buried between commits that add and remove white spaces around the problem, and the last change is just nitpicking around something you should have gotten right in the very moment you posted your PR if only you ran a linter or paid attention to the comments posted in your PR.

Re: PR process killing morale and productivity

#143

Post by chipdart »

philwelch wrote: post #136
chipdart wrote: post #133

Earlier quoted context omitted.

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

> 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 you cannot release a hot fix because your pipeline is broken due to the last commit having 5 spaces instead of 4.

Re: PR process killing morale and productivity

#144

Post by bdangubic »

chipdart wrote: post #131
bdangubic wrote: post #129

Earlier quoted context omitted.

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.

I was just joking but surely I can revert a commit on a feature branch, no?

Re: PR process killing morale and productivity

#145

Post by chipdart »

bluefirebrand wrote: post #117
arp242 wrote: post #98

Earlier quoted context omitted.

You can't "enforce" all of these things. That was my main point. And you absolutely can stop people from doing that. Simply accepting dickish asshole toxic behaviour as "well, people are like that shrug " and never telling people off is exactly the problem.

If you start telling people off at work, there is a good chance that you will be perceived as being the problem If you go to management to complain about a coworkers behavior you may just be told that you have to adjust your expectations to get along with them If there's a team agreement and someone continues to violate it then you actually have a complaint you can make to management that has some bite to it

> If you start telling people off at work, there is a good chance that you will be perceived as being the problem

The whole concept of a PR is to review the changes you ask your team to pull into the repository.

What do you think PR comments are intended to be? Pats on the back and public announcements on how awesome you are?

No, the whole point of a PR is to allow others to review the changes you proposed so that the mistakes you are trying to introduce are easier to spot and prevent.

What do you call comments that flag a problem with your code changes? Do you call it "being the problem"?

> If there's a team agreement and someone continues to violate it (...)

How will they tell if you do not point out those violations in the PRs? That's precisely why they exist.

Re: PR process killing morale and productivity

#146

Post by chipdart »

bdangubic wrote: post #144
chipdart wrote: post #131

Earlier quoted context omitted.

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.

I was just joking but surely I can revert a commit on a feature branch, no?

You can revert and even force-update the branch to rewrite it's history, but GitHub still tracks the old commits and even lists in the PR the events that rewrote the branch history.

Re: PR process killing morale and productivity

#147

Post by Anon1096 »

chipdart wrote: post #126
mixmastamyk wrote: post #121

Earlier quoted context omitted.

Is that new? I don’t remember that feature, working with gitlab a few months back.

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

I'm not sure about gitlab, but on github this is a distinct feature where you post a comment wrapped in three backticks and the language as suggestion. It'll format it as a diff that the author can one-click create a commit from.

https://github.blog/news-insights/product-news/suggested-cha...

Re: PR process killing morale and productivity

#148

Post by whynotmaybe »

> Define (and Stick To) a Style Guide

We have some styling rules that are so much in the "Style Over Substance" category that I have a strong tendency to subconsciously categorize them as "not important, at all". Especially when the IDE doesn't respect them when you use the code generation.

But as it was a strong irritant for some PR reviewers, and as I couldn't force my mind stop doing it, I automated it by writing a custom linter.

Re: PR process killing morale and productivity

#149

Post by The_Colonel »

bluefirebrand wrote: post #115
The_Colonel wrote: post #99

Earlier quoted context omitted.

This: > "Any proposal that requires everyone to just is not a solution, because everyone will not just" invalidates this: > A real solution is to have the team agree on a shared style guide, then enforce it with a linter and formatter. ... since the "team" is everyone. It's basically the same problem. The other issue is that linters/formatters don't "solve" all formatting/stylistic choices. Most formatters, fortunate…

They aren't the same at all A shared style guide is an external impetus to adjust behavior. It comes with external accountability, and also an implicit understanding that violating the shared expectation may bring consequences "Everyone should just be less anal" is expecting an internal impetus to adjust behavior. There no external accountability, and there's no expectation that failing to do so has consequences > Th…

The point where this is the same is "everyone just needs to agree on a common style".

> 80% of a solution is better than 0%

I'm not sure if it's 80% or rather 20%, I guess it's a POV / arbitrary number.

In any case, this is far from a solved problem, while I often see in these type of discussions the idea that auto-formatters solve formatting/code style problem. "Just use black" while dismissing the idea that the rest has to be tackled informally / culturally.

Re: PR process killing morale and productivity

#150

Post by The_Colonel »

wnoise wrote: post #114
The_Colonel wrote: post #99

Earlier quoted context omitted.

This: > "Any proposal that requires everyone to just is not a solution, because everyone will not just" invalidates this: > A real solution is to have the team agree on a shared style guide, then enforce it with a linter and formatter. ... since the "team" is everyone. It's basically the same problem. The other issue is that linters/formatters don't "solve" all formatting/stylistic choices. Most formatters, fortunate…

You're not wrong. But in theory, agreement and buy-in is a one-time thing, while actually writing the code and reviewing the PRs are constant things.

Yes, but aligning the team culture is in theory also a one-time thing. And this is IMHO necessary in any case, no matter if you decide to enforce this alignment using auto-formatters or not.
Post reply on HN