Hacker News Reader

Comment by znpy | Hacker News Reader

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 to do your best work. You will be essentially blocked pending their approval (so that nitpicks are changed according to their likings).

The weird thing is that all this traceability leaves traces for management to see who's doing a good pr review job and who's not... But I've learned that management usually does not care much.

View on HN

It's tough with spelling sometimes.

In my current role I review a lot of PRs, they tend to be large due to the waterfall way things work... If I don't ask to fix the spelling now, It might not happen for a year or two. That said if it can be fixed in separate PR before release, that's fine.

Really, it's best to have some terms or explicitness.

For example, with my teams 'Nitpick' means I'm just being nitpicky, Doesn't have to change unless it's on the edge (and I'm explicit as to why it should change, i.e. I know the next thing will need the change anyway). "Consider" means It doesn't have to happen, but here's some food for thought. "PLZ FIX" is fairly self explanatory.

Also, making sure management (especially for contract houses) knows that PRs are a 'judgement free zone' and should not be held against people for perf reviews etc; that should be collected by other peer feedback channels instead.

Agree. Why did the junior developer feel like their changes were ready to be reviewed. Looks like they had little guidance. Why didn't a senior developer suggest to close the PR, apply the formatting or whatever and then re-open the PR or something like that. 300 comments, that's either a lot of developers commenting or just a lot of back and forth. I don't get how anyone would let it come to that point. Makes me feel grateful for the places I worked at and the experience I have now.

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

I always thought this was the best way.

I wish these systems had a way to assign severity to comments, and urgency to the commit.

If you have a jr developer it is your job to give them stylistic feedback, the problem comes from mixing it in with security holes or sneaky bugs. And when the process doesn’t identify when we need to ship it yesterday, vs in the next few months.

Depending on your company culture, you can also just explicitly write something like "This is not a blocker, but I would suggest...". Of course if your culture uses explicit "press the button to unblock" then that's probably redundant.

Yeah I do similar and encourage colleagues/orgs to do the same.

Half of my comments were "nit: consider blah blah" where I want it on his radar, but I approved anyway and if he declines I'm fine with it.

I like https://blog.danlew.net/2020/04/15/the-logaf-scale/

In many dynamic languages, depending on scope, spelling is not a nitpick, it has long term mental costs and very well can cause bugs.

Spelling automatically should be highlighted by the ide too. Spelling mistakes means you’re blatantly ignoring warnings from the ide.

Please see my other comment. I meant misspelling of English words in the comments. Misspelled code will not compile and/or pass the tests so the pr won’t be accepted anyway.

I should have been less ambiguous: i meant spelling of words on the English language, in comments.

Good pr systems will perform a build of the code and run tests, so if the mis-spelling is in the code the build will fail anyway.

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.

"will approve after nitpicks" is the most asinine behavior

Reply on news.ycombinator.com