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.
- Add a comment
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. 😇
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.
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.
I would make a suggestion but I wouldn’t insist on it being followed, because arguments about style are tedious and rarely productive.
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.
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.
I had to shorten the title, but some of the information you say is missing is actually covered on the question.
Anyway, I just thought of adding this question today because I actually was asked a variant of this in an interview (no mention about code style docs), and the interviewer was not happy with my answer which was something like “Whatever style decision is important should be covered in the style guide. If you don’t have a style guide, then I’d assume that this is not really important for the team, and I rather focus my review on things that really matter. Is the code testable? Is it maintainable? Is the code being introduced completely different from what we have before or are we consistent with our inconsistencies? All in all, I’d rather spend time working on new features and shipping than arguing over style preferences.”
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.
Passive aggressively minify and merge. We’ll sll be unhappy.
