3 ms·
I 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 ag
by punkbit 6mo ago
I 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[].
- Supermancho 6mo agoSame. 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.
- epgui 6mo agoLinters 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.
- __alexs 6mo agoWrite a lint rule. It's often quite easy.
- epgui 6mo agoWhat linter are you assuming I’m running? You can’t always write your own rules.
- punkbit 6mo agoIt's quite simple to write a rule, e.g. for the example I shared https://typescript-eslint.io/rules/array-type https://typescript-eslint.io/rules/array-type. But I've never seen any reputable project arguing against T[] for simple types. In my opinion, that says a lot about the reviewer, not the contributor. If a linter rule doesn't exist, you should argue against the idiomatic convention.
- epgui 6mo agoI’m not arguing any particular rule… But what linter are you assuming I’m running? You can’t always write your own rules.
- james_marks 6mo agoThis 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.
- fishtoaster 6mo agoYeah, 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."