I think it's OK to be direct about problems and suggest specific improvements, provided you phrase them right. For example "this variable name violates our coding standards [link] because [reason]. How about $wibbly_boing instead?" (but not "I see you haven't read our coding standards, this should be $wibbly_boing"). You should also say stuff like "this is great, I wouldn't have thought of doing it that way, hey @everyone
take a look" sometimes.
When I'm writing documentation on working practices I put it something like this: "Code review is not a competitive sport with winners and losers, it is a never-ending conversation aimed both at making improvements but also pointing out best practice.".
As for "I ran this locally" I usually won't even bother reviewing until the automated tests have passed, which makes most of the problem go away. There's still the issue of non-existent (or flawed) tests which mean the code still hasn't been run locally, but that's the sort of thing I'd expect code review to catch. One of the automated checks I like to see (although most places don't have it) is that the test coverage is the same as or higher than the coverage of the merge target. That is again just a guideline, but a dev can expect Questions if he makes the coverage go down!
For further actions, you may consider blocking this person and/or reporting abuse
We're a place where coders share, stay up-to-date and grow their careers.
I think it's OK to be direct about problems and suggest specific improvements, provided you phrase them right. For example "this variable name violates our coding standards [link] because [reason]. How about $wibbly_boing instead?" (but not "I see you haven't read our coding standards, this should be $wibbly_boing"). You should also say stuff like "this is great, I wouldn't have thought of doing it that way, hey @everyone take a look" sometimes.
When I'm writing documentation on working practices I put it something like this: "Code review is not a competitive sport with winners and losers, it is a never-ending conversation aimed both at making improvements but also pointing out best practice.".
As for "I ran this locally" I usually won't even bother reviewing until the automated tests have passed, which makes most of the problem go away. There's still the issue of non-existent (or flawed) tests which mean the code still hasn't been run locally, but that's the sort of thing I'd expect code review to catch. One of the automated checks I like to see (although most places don't have it) is that the test coverage is the same as or higher than the coverage of the merge target. That is again just a guideline, but a dev can expect Questions if he makes the coverage go down!