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

171–180 of 224 posts

Re: PR process killing morale and productivity

#171

Post by thiht »

bsnnkv wrote: post #47

I can't imagine going back to working on codebases in languages that don't come with strongly opinionated defaults. I'm very lucky to spend the majority of my dayjob hours working on Rust code, so everyone just runs cargo fmt && cargo clippy, and this in enforced in CI. You can't even publish a PR until those basic bars have been met. I can't imagine the absolute insanity of working on JS projects where there are mor…

JS has excellent tooling for standardizing code style, Prettier for opinionated formatting and ESLint for everything else. If some teams decide to not use these tools, that’s their mistake.

Re: PR process killing morale and productivity

#172

Post by thiht »

Larrikin wrote: post #168
chipdart wrote: post #122

Earlier quoted context omitted.

> 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

Toxic is a strong word. It’s useless, but not toxic, some people might think they’re making the reviewee’s work easier by flagging all the occurrences of an issue. Maybe they like that on their own PR. Maybe they never thought about it and think thoroughness is expected. If someone’s code review practice is bothering you, just mention it instead of labeling it as toxic.

Re: PR process killing morale and productivity

#173

Post by devjab »

chipdart wrote: post #164
avidiax wrote: post #153

Earlier quoted context omitted.

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…

I’m not sure if I’m understanding you correctly, but how on earth would a pull request even make it to the review state if it fails to lint in the pipeline?

I sort of agree with GP in that the discussions are a waste of time. I also agree with you that you should simply automate it through tools. Styling doesn’t have to be a democracy or about personal preference, all styles work, it’s all about picking one and then forcing everyone to use it. Of course you do it in a much more involving process than what I make it sound like here, but ultimately someone with decision making powers is going to have to lock down the process so no further time is wasted on it.

Re: PR process killing morale and productivity

#174

Post by thiht »

watwut wrote: post #90
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.

IMO, the best way is to have automatic formatter shared by team.

Isn’t that a given in 2024? I find it amazing that it still seems to be an issue in some places. Using a linter/formatter is a no-brainer at this point, and it’s been for years.

Re: PR process killing morale and productivity

#175

Post by thiht »

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.

Strong disagree. If it’s not in the CI, it’s optional. Respecting the formatting standards of the project is NOT optional.

It should definitely be applied as a pre-commit or pre-push hook too to make sure the CI step is just a formality, but it’s not enough.

Re: PR process killing morale and productivity

#176

Post by ilaksh »

The only thing I disagree with him on this is that function length doesn't matter. Longer functions do tend to be harder to understand. I will admit that sometimes a lot of helper functions can make it harder to piece together, so you can overdo it, but usually putting one or two of them back into the main function gives a very readable function.

So I would tend towards more smaller more decomposed functions with descriptive (but concise as possible) names by default. This is very different from old-fashioned C standards, which I feel are very outdated for modern tooling and hardware.

It's definitely important to have formatting standards to avoid arguments about that. But you are never going to completely eliminate stylistic things because part of that is function decomposition or intersects with the actual substantive design. Because some people feel quite differently about function size for example.

Re: PR process killing morale and productivity

#177

Post by bdangubic »

chipdart wrote: post #146
bdangubic wrote: post #144

Earlier quoted context omitted.

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.

sorry for misunderstanding, I am NOT trying to hide the fact I reverted, hence the alias dont-fuck-with-my-branch :) and I was overall just joking…

Re: PR process killing morale and productivity

#179

Post by DontchaKnowit »

lijok wrote: post #63
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…

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.

How are you supposed to write new code without hitting a 3k loc pr?

Re: PR process killing morale and productivity

#180

Post by chiph »

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…

Only one place I have worked at had the goal of writing code as if it were from just one person and it was pretty nice, honestly. The diff tool output was easy to understand (not a lot of noise). What made it work was everyone was pretty mature and understood that this was a group effort, and not everyone was going to get their personal itch scratched around brace style, etc.

We did have some outside contractors who didn't get it at first, but after several of their submissions were rejected (with potential financial penalties) they got onboard and followed the guidelines we had sent them. "These people mean it."

Post reply on HN