alt Hacker NewsI do the same!
But PR discussions about lintable style issues always surprise me. The ideal solution is to add a rule in the linter. But when the team won't agree on the rule, and is open to multiple styles, the author should decide, simple! Had a team mate recently who'd block PRs over T[] vs Array<T>, forcing people to stick with Array<T> for simple types like number[] even though TypeScript's own docs and tooling push T[].
Replies
Yeah, I think all style comments should be handled by either a linter/formatter or be written in a style guide. Everything else is up to personal preference.
Then, when a style comment comes up in a PR, the answer is "Oh, do you think we should add that to our style guide? If so, let's discuss that in slack. Until then, though, that's not blocking."
This is exactly the pedantry I try to avoid in my teams.
The point of a review shouldn’t be to make sure it’s exactly how the reviewer would do it. Is it safe? Does the author understand the area? Is there evidence the code has been exercised? Does it follow the major conventions?
Beyond that, I try very hard to build a culture of approvals vs nitpicking, and let the linters enforce the rest.
Same. I've said my peace, but I don't let that get in the way of what works.
Notably, prevent github merges if there are unresolved comments, so you know they glanced over them before collapsing it.
Linters often don’t provide the constraints you desire. While they’re a great (arguably essential) tool, they often are not sufficient.
When a project has well-established patterns, part of the job is to just follow the pattern, whether you like it or not, until you find a reason to 1) change the pattern everywhere, or; 2) make an exception to the pattern with intention.