[recommended] Remove stylistic rules from recommended #651
Comments
jacobrask
added
package: eslint-plugin
bradzacher
added
recommended-rules
"@typescript-eslint/adjacent-overload-signatures": "error",
style rule.
nobody has complained, so I think it's staying.
- "@typescript-eslint/array-type": "error",
- style rule. settings are very opinionated. should be removed
+ "@typescript-eslint/await-thenable": "error",
+ best practice, should be added.
+ "@typescript-eslint/ban-ts-ignore": "error",
+ best practice, definitely should be added.
"@typescript-eslint/ban-types": "error",
best practice, definitely staying.
"@typescript-eslint/camelcase": "error",
style rule.
It is best practice to enforce some naming convention (eslint doesn't have a snake_case rule).
Most codebases follow this style.
Nobody has complained, so I think it's staying.
"@typescript-eslint/class-name-casing": "error",
style rule.
It is best practice to enforce some naming convention.
Most codebases follow this style.
Nobody has complained, so I think it's staying.
"@typescript-eslint/explicit-function-return-type": "warn",
I would argue this isn't a style rule.
We need to turn allowTypedFunctionExpressions on by default.
IMO should stay in.
- "@typescript-eslint/explicit-member-accessibility": "error",
- As per #201, this will be removed.
- I like it and use it to force devs to think about the exposed interface, but I can understand that a lot of people don't like it and just default to `public` to shut the rule up (defeating the purpose).
- "@typescript-eslint/indent": "error",
- If only because the maintenance on this rule is a nightmare, this should go.
"@typescript-eslint/interface-name-prefix": "error",
this will not be going.
It's not a stylistic thing.
It is an antipattern in typescript that people should not be using.
There's literally no reason to prefix interfaces with `I`.
I will continue to provide reasonable rebuttals against removing this till the day I die.
See #374.
"@typescript-eslint/member-delimiter-style": "error",
style rule.
I think this is good as it at least enforces some form of standard (otherwise I've seen codebases turn into a lawless wasteland).
nobody has complained, so I think it's staying.
"@typescript-eslint/no-angle-bracket-type-assertion": "error",
best practice, definitely staying.
"@typescript-eslint/no-array-constructor": "error",
best practice.
`new Array(1,2,3)` vs `new Array(1)` is very ambiguous and can cause bugs.
this is definitely staying.
+ "@typescript-eslint/no-empty-function": "error",
+ style rule.
+ best practice to avoid unused/useless code.
+ I think this should be added.
"@typescript-eslint/no-empty-interface": "error",
style rule.
best practice to avoid unused/useless code.
nobody has complained, so I think it's staying.
"@typescript-eslint/no-explicit-any": "warn",
best practice to avoid writing unsafe code.
definitely staying.
should stay as a warn so it's not build blocking
+ "@typescript-eslint/no-for-in-array": "error",
+ best practice.
+ I think it should be added.
"@typescript-eslint/no-inferrable-types": "error",
best practice.
avoids writing redundant code.
nobody has complained, so I think it's staying.
"@typescript-eslint/no-misused-new": "error",
best practice to avoid writing incorrect code.
definitely staying.
+ "@typescript-eslint/no-misused-promises": "error",
+ helps avoid unintentional bugs.
+ I think it should be added.
"@typescript-eslint/no-namespace": "error",
best practice to avoid writing code using old features.
definitely staying.
- "@typescript-eslint/no-non-null-assertion": "error",
+ "@typescript-eslint/no-non-null-assertion": "warn",
best practice to avoid writing unsafe code.
definitely staying, but should probably be a warning instead of a build blocking error.
"@typescript-eslint/no-object-literal-type-assertion": "error",
best practice to avoid writing unsafe code.
explicit `as X` casts follow much looser rules than declaration types.
nobody has complained, so I think it's staying.
- "@typescript-eslint/no-parameter-properties": "error",
- style rule. very opinionated. should be removed.
+ "@typescript-eslint/no-this-alias": "error",
+ best practice.
+ there should be no reason to alias this with the currently available language features.
+ I think it should be added.
- "@typescript-eslint/no-triple-slash-reference": "error",
- deprecated - replaced by triple-slash-reference
- definitely staying.
+ "@typescript-eslint/no-unnecessary-type-assertion": "error",
+ best practice.
+ helps ensure you don't have redundant casts and non-null-assertions.
+ I think it should be added.
"@typescript-eslint/no-unused-vars": "warn",
I'm going to leave this because with #688 it should have a much better UX.
- "@typescript-eslint/no-use-before-define": "error",
- stylistic in terms of function declarations.
- variable case is caught by typescript compiler.
- should be removed.
"@typescript-eslint/no-var-requires": "error",
best practice.
esp with `import =` statements, there are few cases to use var requires.
nobody has complained, so I think it's staying.
+ "@typescript-eslint/prefer-includes": "error",
+ best practice. has a fixer. Array#includes is supported in everything but IE.
+ I think it should be added
- "@typescript-eslint/prefer-interface": "error",
- style rule. very opinionated. should be removed.
"@typescript-eslint/prefer-namespace-keyword": "error",
best practice.
nobody has complained, so I think it's staying.
+ "@typescript-eslint/prefer-regexp-exec": "error",
+ best practice.
+ I think it should be added
+ "@typescript-eslint/prefer-string-starts-ends-with": "error",
+ best practice. has a fixer. methods are supported in everything but IE.
+ I think it should be added
+ "@typescript-eslint/require-await": "error",
+ best practice.
+ I think it should be added
+ "@typescript-eslint/restrict-plus-operands": "error",
+ best practice.
+ i think it should be added.
+ "@typescript-eslint/triple-slash-reference": "error",
+ replaces no-triple-slash-reference. Best practice.
+ definitely staying.
- "@typescript-eslint/type-annotation-spacing": "error",
- style rule. very opinionated. should be removed.
+ "@typescript-eslint/typedef": "error",
+ best practice.
+ I think it should be added.
+ "@typescript-eslint/unbound-method": "error",
+ best practice.
+ I think it should be added.
Rules I'm not sure about: Just to complete the list... rules that aren't in recommended and won't be added:
|
bradzacher
added
the
breaking change
|
adding a rule: prefer-const -- it was in |
|
@aladdin-add - non-plugin rules are a separate thing. |
|
Can we add |
Will this be marked as deprecated in the rule itself? |
|
It won't be because it's not being deprecated by the rule.
|
|
I would like to share my thoughts on some of the rules Rules I turn off
Rules I turn on
|
It's stylistic, but everyone seems to be okay with it.
Ordering is just too opinionated to turn on as recommended. The number of configuration options alone show how opinionated it can be!
Yeah I haven't had a chance to use this yet, so I don't have an opinion on it yet! If people think there's value in this rule, then I'd definitely turn it on recommended.
Most of the typescript code I write now a days is functional in some way, so I don't know how valuable this is to people. Happy to turn it on if there's consensus that it is valuable.
Definite value this rule, but it's only just been released AND it hasn't got any options yet. We're planning on having a shorter iteration time for major releases, so this will at least be turned on with the next breaking.
These are set to be turned on! |
There's an option for that though (
Apparently not all of them. |
That case is actually a weird error and not error. It's kind of correct for the compiler to not to flag it compiler. This example is a runtime error because const consumer = () => console.log(variable);
consumer();
const variable = 'foo';However this case is not, because the variable is both defined, and initialised const consumer = () => console.log(variable);
const variable = 'foo';
consumer();So in the first example, the rule will fire, and you will be alerted to a runtime bug. The important detail is where the usage is, not where the definition is. |
|
Right, but my point was that users might still want to use the rule for these cases. |
|
Also note that the original eslint recommended rules also don't include |
|
I think I've already given my 2 cents, but the argument to keep formatting/casing rules here seems to be it's a "Best practice" and "Nobody has complained" but the readme explicitly discourages feedback about subjective rules like this:
I guess I simply don't think it's TypeScript's or a TypeScript specific eslint config's concern which best practices regarding casing I follow, even if I prefer to enforce it it most projects. Would you be open to having one (Really hope I don't come across as hostile or negative by the way, this is a great project and differences in opinions here is normal and happens all the time, just trying to include another perspective in the discussion 👍) |
I never assume ill intent. I appreciate everyone giving their feedback! The reason behind that big paragraph in the config readme is because when we implemented the recommended set, we got a lot of people who raised issues about the recommended set, and they essentially just said "I don't think this rule should be recommended because I don't use it, the end". Which (a) just causes noise and spam for everyone watching the repo, and (b) isn't a good argument at all, right? Since adding that readme, we've essentially received zero issues that have that non-argument. Instead we've received a number of well thought out requests along the lines of "I don't think This gives the community something to chime in on and discuss, and it gives us, the maintainers, something to think about and consider. There are a few rules which are being removed because of these discussions. The wording in that paragraph is probably a bit strong, as I was writing it when I was pissed off about getting another issue, but the points within it are valid. I'm hesitant to change it because I don't want to encourage too much discussion about the recommended set. It's such a teeny, tiny fraction of this project, yet it already takes up such a huge amount of my time. Our stance with the recommended set is to give people the best practices by default. This includes rules which are considered "stylistic" rules. The best practice and standards within the community include some stylistic decisions like naming for classes/variables. We want it to be setup such that a user that is new to typescript can install eslint + our plugin, drop the recommended set in, and instantly have best practices enforced on their codebase. Ultimately it's very easy to adjust the config if you like a different convention. We could definitely add an additional, looser recommended config. A number of people have suggested it, but nobody has wanted to do the work to create it. |
Isn’t this what I think the I think that by calling the more opinionated, stylistic rules “recommended,” this repository contradicts the spirit of the |
|
Are you using I think I'm misunderstanding parts of your post because I'm not sure which one you're using. I think you've misunderstood the intention behind our eslint-recommended config. It is not a "looser set" of rules - if it was, then it would turn on a bunch of our rules, which it clearly doesn't. It's intended to be a list of rules which don't need to be implemented when you're using typescript, because the typescript compiler already reports errors for them. Really the only time you'd want those rules enabled is if you are using babel, which doesn't do type checking.
We do, and these stylistic conventions haven't just come out of nowhere. The recommended set in v1 was based on combining 3 community maintained configs together, whilst also taking into consideration what the microsoft does in their codebase, as well as what standards are used in the official typescript documentation. We know that eslint's recommended set purposely doesn't have any opinionated stylistic rules. Their intention is to just onboard people with a small set of rules which help catch problems at lint time, because vanilla JS has no checks otherwise. We have chosen a slightly different brand for the recommended set, as I mentioned:
There is no documented guidelines that I can find which explicitly state that recommended sets should only do X. It's up to the plugin author as to what they recommend. |
My bad. My usage in the comment above:
I do not discuss
Again, more evidence in favor of the idea that calling opinionated rules “recommended” is surprising. The distinction between
This is a fair point, but I still think calling “best practices” “recommended” is confusing, given its de facto usage by eslint. I would consider calling the ruleset In my opinion, inclusion into a config called
This might sound strict, but I believe setting a high standard is good insofar as it reduces configuration load for people starting new projects. This excludes almost every rule which is describe above as being “a best practice,” but I don’t think this is a bad thing insofar as it provides a solid base to build upon which doesn’t mislead users. At the same time, typescript provides new opportunities for rules that catch more bugs because we have type information. Some examples of rules which I think merit inclusion based on the above conditions:
|
I mean, it's not subtle at all, and we have documentation which clearly documents their usage and their intention: In addition
So for example we have our version of You're saying that we shouldn't turn that on, even though it'll prevent you accidentally writing code which generates a bunch of literally useless runtime code, because eslint base doesn't recommend it baed on the fact that it doesn't handle the case we can?
I fail to understand how we're misleading users. What is the definition of recommended? "advised or suggested as good or suitable". The "convention" of only providing a minor subset of rules is only strictly followed by eslint, but it isn't documented anywhere outside of tribal knowledge. And no doubt, they do this purely because people are so vehemently opinionated about styling that the issues would create noise from a userbase of ~7mil weekly downloads. The typescript community is a lot more standardised. We know this because we've seen it - people have followed the lead of companies like Microsoft (from their typescript repos and documentation) and Google (from their angular2+ project and documentation). We are telling people that these stylistic standards are a good thing to use because there are industry leaders using them. Ultimately there's no requirement to use the recommended set, nor is there a requirement to provide zero config within your local config if you don't agree with our suggestions. |
Let me provide a succinct argument for why calling a more opinionated ruleset
This argument has nothing to do with the actual English definition of “recommended” and everything to do with conventions established both by eslint and this repository itself. As programmers, anytime we can intuit or establish naming conventions, we have a prima facie obligation to do so insofar as this adheres to the “principle of least astonishment.” This argument also has litle to do with the way other eslint plugins apply the word “recommended.” Sure, other eslint libraries apply more opinionated rules in their ”recommended” ruleset and have on occasion received a lot of flak for doing so (I’m looking at you Now let me describe a typical user journey given the current config names:
Friends. I have a confession. I am the developer described above. I also believe this is probably a distressingly common user journey. Ultimately, what I was looking for was a config which stopped the false positives caused by
Let’s run
Not really. Even if a developer accidentally refactors an async function to a sync function, the typescript compiler is already really good at telling the developer when they use a function’s return value in an unexpected way. I don’t think this rule catches bugs which aren’t already caught by the type checker.
No. There are many reasons why a developer would write an async function which does not contain
No. You’re saying that this rule could be enabled because it catches additional cases thanks to typescript-eslint’s typechecking abilities, but I don’t think handling “an async function with no await which returns a promise” somehow prevents this rule from catching the cases described in test 2. Indeed, I was looking at why this rule even exists, and it’s because there’s another typescript-eslint rule (promise-function-async) which forces you to prefix functions which return promises with In any case, the fact that you’re adding this rule to Let’s not get lost in the weeds about specific rules. Ultimately my objections aren’t about any specific rule and more about naming, which is an important part of our jobs as programmers. There is no reason why you couldn’t name this more opinionated ruleset @bradzacher |
Stylistic rules can be very useful to set a standard that's consistent in your code base to help people learning and move between projects, but they make it a more difficult to transition an existing code base, and also makes TypeScript less approachable to beginners. I've seen StackOverflow questions with people writing "Please help me fix this TypeScript issue, I can't commit!" when it's actually just a stylistic lint error.
For this reason (I assume), the default
eslint:recommendedconfig is quite slim, and it would be nice if@typescript-eslintfollows a similar standard for what's recommended.Even just following the current documentation for the recommended rules would be a nice start 😃
Stylistic rules
I think these rules should be removed from
recommendedwith the 2.0 release. They do not have any effect on the runtime behaviour or type checking of your code (e.g. do not catch issue vectors in your code, and I would not consider something like specific indentation to be a TypeScript best practice)@typescript-eslint/array-type@typescript-eslint/camelcase@typescript-eslint/class-name-casing@typescript-eslint/indent@typescript-eslint/type-annotation-spacing@typescript-eslint/interface-name-prefix@typescript-eslint/member-delimiter-style@typescript-eslint/no-inferrable-types"Best practices"
These could be considered best practices by some, but are still quite opinionated so it would be nice to take 2.0 as an opportunity to consider if all rules really are suitable for
recommended.@typescript-eslint/explicit-member-accessibility- I personally use this in most projects, but accessibility is optional in TypeScript for a reason so it's more of a matter of preference and opinion rather than a general "TypeScript best practice"@typescript-eslint/explicit-function-return-type- SettingallowTypedFunctionExpressions: trueas default will help, but it is very unergonomic to use right now.@typescript-eslint/no-array-constructor- corresponding rule is not enabled in eslint-recommended@typescript-eslint/no-non-null-assertion- the ! operator is a useful escape hatch, having to workaround it leads to a lot more code that is more difficult to understand@typescript-eslint/no-object-literal-type-assertionI hope that a change like this would A) help drive adoption of the project and the very important and relevant rules that would still be left in
recommended, B) avoid driving away beginners that just select "defaults" / "recommended" from the TypeScript ecosystem by overly strict rules that are mostly relevant in some specific code bases.Thanks!
The text was updated successfully, but these errors were encountered: