Experienced Devs

I would like to add to the other excellent points on when to avoid pressing the matter.

If the person has multiple reviewers, and the review itself is far along enough that most of the complex comments are fixed then it is quite irritating having new comments that focus on style.

Giving a comment on style is to be avoided late into the review. Some reviews even have very long running CI/CD that reruns on every commit and won't accept older results. If it makes the assignee wait for even 4 more hours it's not worth it to give such comments after everything else.

It depends very much on whether it violates SOLID. I do strongly encourage my teammates to use new language features when appropriate, but I would never fail a PR for it, just mention "here's a better way to do this in the future."

But if someone is violating the architectural integrity of the application, that's something else. If a field isn't final when it could be, or is public when it could be private, or uses a class that technically works but is intended for a different purpose, or just adds unnecessary complexity, I won't approve those until they are addressed.

That being said, you do have to pick battles. The current app I'm working on is a bit of a mess, architecturally, and sometimes I'll work with someone to try to adopt better standards and it turns out doing something the right way is just too far afield. Just this week I spent an hour trying to help someone get it right before conceding we should just do it quick and dirty and address the structural problems in a more deliberate way once the underlying structure was ready to support that.

Coding is as much art as science. Sometimes we like code to look different, but at the end of the day if something is isolated and passes tests, it's fine for now and can be rewritten in the future should that become necessary.

I would make a suggestion but I wouldn't insist on it being followed, because arguments about style are tedious and rarely productive.

Not enough information.

New team member? Show them the style guide and where it doesn't match.

Is the style guidelines consistently followed elsewhere? If not, I'd just approve it.

Do I have a good relationship with the other developer, and can they handle criticism? If not, I probably would not want to be the one reviewing it, but if I did I would likely let it go and fix it later. Fight more important battles.

Otherwise, how important is that piece of code? I'd immediately approve a one-off script, but if it's an core piece of code, I assume the dev missed it and point it out. Happens to everyone.

etc. etc.

The link refers to it being a style in an area that isn't covered. In that case I'd favour consistency with the current code it's touching and open an issue to update the style guide to cover the new area.

If a style guide exists, then style is important enough that this needs to be discussed with the team if it's an edge case that isn't covered by the guide.

Not a programmer. Just love documentation. 😇

I would allow it and approve the PR, then I would ask the team lead if he wants to schedule time to update the style guide to cover cases like this, and set up a linter which can ideally autofix any existing code that violates that rule. Not everyone will agree with the rules set by a linter, but they ensure style consistent, and, more importantly, save everyone time and drama related to inconsequential differences in opinion (at least inconsequential to the bottom line).

I am interpreting "don't particularly favor" as not particularly significant nor important for me personally nor in general.

• Add a comment
  • explaining, in subjective wording, why I prefer it differently
  • adding a change suggestion that may or may not be applied to demonstrate it
  • a personal conclusion and assessment, how important or significant it is to me, and what's fine by me even if not my preference; e.g. "pick what you prefer or want" or "consider change effort"/"fine for now" or whatever

I feel like "sending back to the owner" sounds too strong; although it practically is that, it is not about sending it back; I share my opinion and view, and let the owner decide at their discretion

Whether I suggest a guideline depends on what it is; whether consistency and conformance would be useful in such a case, not for personal, subjective preference, but for code consistency or because they end up preferring my suggestion too.

If it's a problem I might do something about it, but I doubt my personal stylistic preferences should determine that of a shared project.

Depends on how strange and impactful their choice is.

If it's something that I think should be in the style guide, I'd promise try to achieve consensus. I'd prefer not merging in the dubious code because then other people may take it as precedent.

One guy really wanted to write his code differently than the existing code and how others were doing it. It kind of sucked. Not that his way was bad, but no one else on the team subjectively liked it. I relented and let it go, and then had to deal with that unpleasant code for months. Eventually he moved on and a lot of that code got replaced. I retrospect I would have preferred if we had somehow convinced him to keep in the style we preferred. I'm sure he wasn't happy that the rest of the team wasn't keen on his style choices.

If it's just a little weird, mention it as a non blocking comment. Like one guy would have weird line breaks in his longer comments. It technically followed the guide (under max line length) but it was weird. We asked him to stop, he said ok, no problem. But I didn't block a merge over it.

Passive aggressively minify and merge. We'll sll be unhappy.

Depends on the wind, really.