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

181–190 of 224 posts

Re: PR process killing morale and productivity

#181

Post by BeFlatXIII »

EliRivers wrote: post #93
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.

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.

Re: PR process killing morale and productivity

#182

Post by 1123581321 »

Brian_K_White wrote: post #116
Jare wrote: post #97

Earlier quoted context omitted.

> it will become far less obvious that their comments should be downgraded. I'm curious why you think this would happen... I would imagine that their comments will now have be to about things that matter, and if they are unhelpful they will stand out more.

It is obvious that a style comment is of no significance, and that the person who made it chose to spend time and personal capital on something of no significance. It is not obvious that an actual code change is of poor quality. To show that takes infinitely more experience, work to analyse all the direct and indirect ramifications of both the original and proposed approaches, capacity to push back and decide that yo…

If the seniors are too passive to say no faster and faster, and management is absent, the junior’s going to end up being the tech lead. I’ve seen this. :)

It’s not necessarily even bad if there is no meaningful leadership above or alongside the junior. The fast-rising junior may end up hiring developers who are more assertive and still capable.

Re: PR process killing morale and productivity

#183

Post by rudasn »

Leherenn wrote: post #69
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…

Something helpful I have seen in this situation is to require the reviewer to test the changes for long PRs, and post proofs (a screenshot, logs, ...). Even with the best of intentions, it's easier to miss things in longer PRs, plus it gives more time for others if they want to have a look. It's not bulletproof of course, but at least it makes sure the basic functionality is working.

Agree. I've made it a habit the past few years to include before/after screenshots in my PRs and sometimes a screen capture of the UI/workflow changes. Also, screenshots in the issues pages for bugs or anything UI related.

I find it makes it easier for people to see and understand the goal of your changes and helps them focus on the how in the code review.

When others do that in their PRs I can jump straight into the tests files and get a better understanding of the what is proposed (or at least start the review process in a more substantial way - get the important stuff addressed early on).

Re: PR process killing morale and productivity

#184

Post by jessekv »

greatgib wrote: post #157
herpdyderp wrote: post #39

Earlier quoted context omitted.

This kind of feedback has largely been solved, for me / my team at least, with linters and formatters (as mentioned in the article). So I'd say it is still a reflection on the code review process being broken.

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.

Re: PR process killing morale and productivity

#185

Post by plasticchris »

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…

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.

Re: PR process killing morale and productivity

#186

Post by mihaaly »

pavel_lishin wrote: post #138
mihaaly wrote: post #107

Earlier quoted context omitted.

Who cares?!

I care. I don't particularly care which one you choose - although I admit to having a preference - but I want that style consistently applied throughout the project, so I'm not stumbling over stylistic differences as I'm scrolling through code trying to investigate something. And keep in mind that this particular example, that some people can read clearly, is just five lines. When you're looking at a 3k line file, on…

You need to work with code from multitude of sources, then you will learn how to read code efficiently. And how futile are your dreams about styles. We are not supermodels and fasion designers, far from it, get over it!

Re: PR process killing morale and productivity

#187

Post by mihaaly »

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…

The trouble is with those rush code out without really looking what they are doing. They make those mistakes, very easily, indeed.

No style could save us from those devils!

People should take the necessary attention to make quality work (and use proper techniques, where proper does not entail style or other visuals or appearances, not at all. That's for superficious persons). This is true for any part of life not just coding.

Re: PR process killing morale and productivity

#188

Post by Aeolun »

Where do people find these places? I have a hard time getting management to accept that more restrictions on who can review is not a bad thing.

Nearly 90% of our PR’s have zero comments.

I do have a hard ban on formatting comments though. If it’s not covered by the linter/formatter everything goes.

Re: PR process killing morale and productivity

#189

Post by Aeolun »

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 very much dislike any process modifying my commit after I submit it. Rebasing becomes hell.

Re: PR process killing morale and productivity

#190

Post by Aeolun »

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.

Sorta, but it’s almost never worth blocking the PR over. Just ask to fix and approve. If it doesn’t happen and you come across it later just fix it.
Post reply on HN