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

211–220 of 224 posts

Re: PR process killing morale and productivity

#211

Post by camgunz »

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…

The answer is to require a high percentage of test coverage. Then you skim through the code to get a general idea of what it does and how, then scrutinize the tests to verify they enforce that understanding.

This does three things. First, it prevents people from dropping big chunks of code and asking their reviewers to make it work (if coverage is too low, it doesn't work). Second, the tests spare you the cognitive work of verifying the code does it's supposed to--especially edge and failure cases. Third, this frees you up to think about the bigger picture. Is this the right way to do this, should we be doing it at all, does this conflict with something else in the works, etc

Re: PR process killing morale and productivity

#212

Post by EliRivers »

BeFlatXIII wrote: post #181
EliRivers wrote: post #93

Earlier quoted context omitted.

have the reviewer just adjust it themselves Passive-aggressive minefield. Someone will see a change done to THEIR code, without the changer even asking, and it will feel like the person who did it is a passive aggressive dickhead. Resentment will brew, tempers will be lost. It will only get worse from there. Software engineers already aren't exactly known for their humbleness and ability to swallow their ego.

Sounds like a team of overvalued prima donnas.

Oh, so you've met software engineers? :)

Re: PR process killing morale and productivity

#213

Post by rurban »

chipdart wrote: post #123
OptionOfT wrote: post #80

Earlier quoted context omitted.

This is why I love things like cargo fmt / go fmt / eslint / etc. No discussions of whether if (foo) return; is valid, or we should do if (foo) { return; }

> This is why I love things like cargo fmt / go fmt / eslint / etc. I agree. Once I had the displeasure of working with a junior dev who was very prolific in posting comments on style and if a space should be at the left or at the right of a symbol. It took me a few days of dealing with that noise to onboard a linter. Even so the junior dev felt entitled to manifest how high their standards were by posting a torrent…

Install pre-commit with various linters for all the languages and data files out there, and the problem is solved. I do it even for Makefile's and cmake.

Add it to your CI also, make lint and a make fmt.

Re: PR process killing morale and productivity

#215

Post by philwelch »

chipdart wrote: post #210
philwelch wrote: post #208

Earlier quoted context omitted.

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

Linters aren’t only for checking style issues.

Re: PR process killing morale and productivity

#216

Post by cempaka »

plasticchris wrote: post #203
Aeolun wrote: post #189

Earlier quoted context omitted.

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

Yeah we just have any linter failures fail the CI build, but it's left up to submitters to decide how to resolve the violations.

Re: PR process killing morale and productivity

#217

Post by plasticchris »

cempaka wrote: post #216
plasticchris wrote: post #203

Earlier quoted context omitted.

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

Yeah we just have any linter failures fail the CI build, but it's left up to submitters to decide how to resolve the violations.

Yeah, that’s what I meant in the first comment :)

Re: PR process killing morale and productivity

#218

Post by Mawr »

meowfly wrote: post #124
mihaaly wrote: post #107

Earlier quoted context omitted.

Who cares?!

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.

Of course you can, it's a tiny, isolated example consisting of 5 or so tokens. That's not how real programs look like so you cannot take the example so literally.

The second form is superior for at least three reasons:

1. Enables a property corollary to the happy path rule, which is that every return statement of a function is at the beginning of a line. This property is crucially important for readability, it makes it reliably possible to determine every exit point of a function by merely scanning the left hand vertical slice of it as opposed to needing our eyes to jump around erratically searching for arbitrarily placed returns.

2. Uniformity: mixing different if statement styles in a single codebase interferes with our ability to match visual patterns. Since we can no longer rely on every if statement looking the same, we cannot quickly skim over code anymore.

3. Refactorability: it's easier to add another line of code to the if statement if there's no need to also add brackets.

Re: PR process killing morale and productivity

#219

Post by move-on-by »

plasticchris wrote: post #185
chipdart wrote: post #123

Earlier quoted context omitted.

> This is why I love things like cargo fmt / go fmt / eslint / etc. I agree. Once I had the displeasure of working with a junior dev who was very prolific in posting comments on style and if a space should be at the left or at the right of a symbol. It took me a few days of dealing with that noise to onboard a linter. Even so the junior dev felt entitled to manifest how high their standards were by posting a torrent…

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 agree that it should be hooked up to run as a precommit hook, but unfortunately that doesn’t always solve things. People can- and do- bypass precommit hooks. It happens all the time at my company with certain teams, but I’ve been unable to figure out why. Any precommit hook that should run- needs a correlating required PR action to verify any precommit expectations are met. Otherwise, people reviewing the PRs just assume it ran and the people bypassing it get away with it.

Re: PR process killing morale and productivity

#220

Post by cced »

move-on-by wrote: post #219
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 agree that it should be hooked up to run as a precommit hook, but unfortunately that doesn’t always solve things. People can- and do- bypass precommit hooks. It happens all the time at my company with certain teams, but I’ve been unable to figure out why. Any precommit hook that should run- needs a correlating required PR action to verify any precommit expectations are met. Otherwise, people reviewing the PRs just…

Create a linting step that fails if running the linter creates a git diff delta.
Post reply on HN