This is a challenging balance to strike. I used to work with someone who I considered "that guy", but there was no denying that the code ended up better for it, even if it would have been basically fine otherwise. Especially over the long term, those little things can add up.

On the reviewer side, I have sometimes been in the situation where I end up letting something slide because I didn't want to be "that guy". Then I end up reading the code later and needing to fix it.

I think the answer here unfortunately relates to power dynamics. I'm senior, and others believe I'm senior, so when I dismiss reviews like this, no one fights me. Sometimes, with folks like this, even without being senior, just standing up to them works.

As others have said, using linters, auto-formatters, etc often helps with this as a lot of the things these people like to nitpick are then standards enforced by code, and you can send them off to fight people about the standard ("but not in this PR").

Two things, first have a standard to compare to. Second, use code linters and formatters to automatically enforce it.

That obviates trivial back and forth, leaving the interesting issues.

You'd be surprised. Often that dude will have preferences that go way beyond linters and may even contradict widely accepted best practices.

I've worked with the dude who insisted on factoring out functions for unrelated 2-liners, creating extra coupling where we don't want it, the dude who insisted on mocking library code in tests when it would be easier and more thorough to just use it and make sure we're calling it right..

My favorite was the dude who, after 3 days of back and forth changes on a semi-urgent PR goes silent for a day, I ping him, he says "thinking of more comments :)" with the goddamned smiley.

Hah! The simple answer to that however, is "No." With a link to standards/best-practices if you want to be polite.

Unless dude is your boss, in which the answer is "Yes," and the paycheck arrives on Friday.

Also, sometimes other people are right... something to remind myself of. ;-)

Yes, or the guy who wants you to mock out database calls because it's not a "real unit test" otherwise. So you spend another day mocking out crap when it's both easier and less error-prone to use the real thing. I guess the time spent will pay for itself... in 10 years.

The point here is to be able to run the tests without a database, and in this case they're probably right. Saying it's for the sake of purity of the test type is wrong, but you should mock out database calls in unit tests because you want them to run fast and without dependencies.

Move those tests to integration...

We don’t run tests without a database. Never been anywhere that did. I greatly prefer integration tests, in general. They almost always have value.

90% of people are fine and code review has tons of benefits.. but the format is an enabler for 'that guy', as you're asking them for approval rather than running it by them as an FYI.

I've seen a few different motivated hard workers get to hard work on demanding as many changes as possible whenever they are on a CR.

There is also the flip side too where a block of code might just have a lot of problems and I’m trying to evaluate if it’s too hard in a public forum to flag all of them at once. If possible I try to flag a couple and to sit down and offer to do a one on one for more.

+1 to taking it offline when it's more than a couple minor style fixes.

I think it all comes down to judgement and there are definitely ways people can write code in their individual style without hurting maintainability.

That said, maintaining a codebase where there is a lot of variation in idioms, style and formatting can be a big headache.

It adds cognitive overhead and if you're changing files you constantly have to balance preserving the local style with other factors. E.g. if someone likes aligning their equal signs, and you add a new line where the LHS is one character longer, do you reformat the whole block?

I can't fathom to be honest to be back in a place where something like that is even a question.

We use spotless. We use prettier. Nothing passes the build if you don't adhere to the style rules. You may disagree with some stuff. Make a case! Change the rule definition, apply them and show the PR to a representative cut of the developer community. You might get it through. We've done a few like that in my time here. Or you might realize what that rule change really means in some cases and the PR is ultimately declined. Has also happened. Ruled looked good for the place that triggered the want. Sucked in most other places, so we decided not to do it.

If you have a guy that can't live with not getting his will 100% of the time my advice is to try and get rid of him as soon as you can. Or run yourself. Not healthy in the long run and frankly "I'm too old for that shit. Now get off my lawn"