Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Suggestion: standardize against prefixing rule names with "no-" #203

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
JoshuaKGoldberg opened this issue Feb 3, 2019 · 15 comments
Closed

Suggestion: standardize against prefixing rule names with "no-" #203

JoshuaKGoldberg opened this issue Feb 3, 2019 · 15 comments
Labels
accepting prs Go ahead, send a pull request that resolves this issue breaking change This change will require a new major version to be released package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@JoshuaKGoldberg
Copy link
Member

Many ESLint and TSLint rules start with "no-" to indicate a preference against some behavior. Some behaviors are always bad (e.g. no-eval), but others eventually are requested to have a preference towards. Having "no-" starting the rule name then makes it weird to add an option to encourage the behavior. For example:

Having only some rules start with no- also makes them a little less discoverable. There are a few more characters to scan through, and alphabetical lists make a little less sense.

Since it's already a conversion to go from TSLint to ESLint, can there be a preference set for not starting rule names with "no-"? In the two cases above, it'd be better if they were parameter-properties and type-alias.
@JoshuaKGoldberg JoshuaKGoldberg added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Feb 3, 2019
@armano2
Copy link
Collaborator

armano2 commented Feb 3, 2019 •
edited
Loading

no- prefix in eslint is used for rules which disallow / disable some functionality.

Changing names of rules is considered breaking change and can be done only in major release

I don't disagree that some of rules should not be prefixed with no-, if options allow to inverse ( enforce) functionality.

https://eslint.org/docs/rules/

@JoshuaKGoldberg
Copy link
Member Author

Changing names of rules is considered breaking change

How about rule aliases? As in, having a no-parameter-properties file that just re-exports parameter-properties? At the next major version it could log a deprecation warning, and a major version after that, log an error, and one major version later, stop existing.

Just to clarify: for rules that aren't yet ported, would removing the no- be acceptable?

@armano2
Copy link
Collaborator

armano2 commented Feb 3, 2019

eslint provides api for deprecation and replacing rules

  • deprecated (boolean) indicates whether the rule has been deprecated. You may omit the deprecated property if the rule has not been deprecated.
  • replacedBy (array) in the case of a deprecated rule, specifies replacement rule(s)

https://eslint.org/docs/developer-guide/working-with-rules#rule-basics

@armano2
Copy link
Collaborator

armano2 commented Feb 3, 2019 •
edited
Loading

I don't have "final" answer for you now about usage of prefix.

my opinion is that we should follow eslint rule naming convention

https://eslint.org/docs/developer-guide/working-with-rules#rule-naming-conventions

The rule naming conventions for ESLint are fairly simple:

  • If your rule is disallowing something, prefix it with no- such as no-eval for disallowing eval() and no-debugger for disallowing debugger.
  • If your rule is enforcing the inclusion of something, use a short name without a special prefix.
  • Use dashes between words.

@typescript-eslint/core-team what is your opinion on this?

@armano2 armano2 added question Questions! (i.e. not a bug / enhancment / documentation) and removed triage Waiting for team members to take a look labels Feb 3, 2019
@bradzacher
Copy link
Member

Yeah we need to knuckle out some guidance around this. Most of our no- rules were built because someone had an opinion about a ts feature and nobody had an opinion about ever wanting that feature on!

Now that we are much bigger, we should definitely nut out some guidance docs around this. no- rules only work in rare cases where it's bad to use a feature, or it makes no sense to have an inverse check (as in no-eval or no-explicit-any).

We can deprecate and redirect users to new rules to replace some of the no- rules without doing a major bump, but removing them obviously is breaking.

@Jessidhia
Copy link
Contributor

Note also that you can't just rename the rule from no- to "no no-". You also have to flip what "always" / "never" / et al mean.

Not saying it wouldn't be a good change, it's just not simple to do, and can't really be done with aliases unless there is some refactoring done.

@mysticatea
Copy link
Member

mysticatea commented Feb 4, 2019 •
edited
Loading

Just I mention about the deprecation policy in ESLint core.

  • Deprecating a rule happens in a minor version.
  • Deprecated rules are frozen. People can continue to use the deprecated rules, but they don't change the deprecated rules even if bugs exist.
  • They make renaming a rule in a minor version if it's needed because renaming is a combination of deprecating a rule and adding a new rule. (it's not alias.)
  • Some precedents exist in core. For example, no-comma-dangle had been renamed to comma-dangle because it's a rule that enforces the style about trailing commas rather than that disallows trailing commas.

Related references:

@j-f1
Copy link
Contributor

j-f1 commented Feb 4, 2019 •
edited
Loading

Should we do a 2.0 and get rid of all the no- rules? Since we don’t have the support guarantees of ESLint core, we should be OK to just remove them.

@bradzacher
Copy link
Member

A number of the no- rules make sense as things that won't be turned on and have no inverse, so a blanket rule against no- prefixes is a bad idea.
I agree though that it makes sense for us to advise against that naming if it's feasible that we will add inverse functionality to a rule.

Going through the list (checkmark = imo should be renamed):

  • no-angle-bracket-type-assertion
    • IMO shouldn't have an inverse because the <cast>value syntax is essentially deprecated.
  • no-array-constructor
    • has no inverse
    • maybe should be renamed though because it sounds like you shouldn't use any array constructors, when it's only enforcing correct usage.
  • no-empty-interface
    • has no inverse
  • no-explicit-any
    • has no inverse
  • no-extraneous-class
    • I doubt anyone would ever want the inverse of this unless they absolutely love java.
    • would be good to rename this so it's clearer though (no-class-as-namespace?)
  • no-inferrable-types
    • could feasibly have an inverse but I don't think it makes sense to have one
  • no-misused-new
    • has no inverse
  • no-namespace
    • would people want an inverse for this? I haven't ever seen namespace used outside of definition files.
  • no-non-null-assertion
    • could have an inverse? (would require typechecking though)
  • no-object-literal-type-assertion
    • inverse seems weird here due to the type-unsoundness which as X introduces?
  • no-parameter-properties
    • needs inverse
  • no-this-alias
    • has no inverse
  • no-triple-slash-reference
    • has no inverse (because /// <ref /> is essentially deprecated)
  • no-type-alias
    • needs inverse
  • no-unnecessary-type-assertion
    • has no inverse
  • no-unused-vars
    • has no inverse
  • no-use-before-define
    • has no inverse
  • no-useless-constructor
    • has no inverse (well technically it does - force every class to have at least a useless constructor, but that seems like a really poor practice).
  • no-var-requires
    • has no inverse that makes sense in TS.

@armano2
Copy link
Collaborator

armano2 commented Feb 4, 2019 •
edited
Loading

i don't see point in rules like:

  • no-non-null-assertion: inverted -> what it will check? that you need null assertion?
  • no-type-alias: inverted -> there should no be inverse, we should have 2nd rule: prefer-type-alias/no-simple-interfaces or something like that

it's better to keep rules simple

@bradzacher
Copy link
Member

the problem with two rules is now you can have a state where you're both not allowed to use interfaces and not allowed to use type aliases.

we can have a rule type-definition-style which has an option to pick either interface or type-alias. It's still simple and prevents invalid states.

@bradzacher bradzacher mentioned this issue Feb 5, 2019
Closed
@JoshuaKGoldberg JoshuaKGoldberg mentioned this issue Feb 9, 2019
Merged
4 tasks
@gavinbarron
Copy link
Contributor

Regarding the change to add options to explicit-member-visibility in #214 I guess the better name for the rule would be member-visibility I assume that this would however be a breaking change.
What's the process/plan for implementing these changes as a whole?

@JoshuaKGoldberg JoshuaKGoldberg mentioned this issue Apr 3, 2019
Merged
@bradzacher bradzacher mentioned this issue May 8, 2019
Closed
14 tasks
kaicataldo pushed a commit to kaicataldo/typescript-eslint that referenced this issue Aug 27, 2019
@bradzacher @JamesHenry
[FEAT][BREAKING][1/2][member-delimiter-style] Better handling for sin…
3786060 
…gle line (typescript-eslint#203)

This PR adds better handling for single line delimiters, and better enforces the `requrieLast` option.

Fixes typescript-eslint#92

## Single line delimiters

Previously single line delimiters were either checked or not. This meant that if you used
```
{
    delimiter: "none",
    ignoreSingleLine: false,
}
```
then the fixer would break your code by removing the delimiter from single line statements i.e.:
```TS
interface Foo { bar: string, baz: number }
// fixed to this broken line
interface Foo { bar: string baz: number }
```

### Breaking change:
- Removed option `ignoreSingleLine`
- Added option `singleLine: "none" | "semi" | "comma"`

`singleLine` works the same as `delimiter`, except for single line statements only.

## Stricter `requireLast`

Previously `requireLast` did nothing when turned off, and enforced having a delimiter when turned on.

I felt this was inconsistent with the goals of the linter; being a tool to provide consistent coding conventions.

This is the first of two changes to bring the option into line with eslint's [`comma-dangle`](https://eslint.org/docs/rules/comma-dangle#options). (The next change will replace the boolean with an enum; I didn't want to overload this PR).

### Breaking change:
- `requireLast: false` now enforces that there is no delimiter on the last member.
@JoshuaKGoldberg JoshuaKGoldberg mentioned this issue Feb 22, 2020
Merged
@JoshuaKGoldberg JoshuaKGoldberg added accepting prs Go ahead, send a pull request that resolves this issue breaking change This change will require a new major version to be released and removed question Questions! (i.e. not a bug / enhancment / documentation) labels Mar 8, 2022
@JoshuaKGoldberg
Copy link
Member Author

Marking as accepting breaking change PRs per the list in #203 (comment).

@7heQuietOne
Copy link

would like to add that some "no-" rules' names don't lend themselves well to understanding; for example, "no-inferrable-types" can (and has multiple times on my team) give the impression that it does the opposite of what it does. It has been floated that "force-type-inference" would be a good disambiguation.

@JoshuaKGoldberg JoshuaKGoldberg added this to the 6.0.0 milestone Jul 25, 2022
@nickserv
Copy link

Can we at least keep the no- prefix for extension rules? I don't think the benefits of simplifying the APIs apply to rules that are already in ESLint, and this makes migrating from ESLint rules more confusing.

@typescript-eslint typescript-eslint locked and limited conversation to collaborators Nov 17, 2022
@JoshuaKGoldberg JoshuaKGoldberg converted this issue into discussion #6022 Nov 17, 2022
@JoshuaKGoldberg JoshuaKGoldberg removed this from the 6.0.0 milestone Feb 22, 2023

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Assignees
No one assigned
Labels
accepting prs Go ahead, send a pull request that resolves this issue breaking change This change will require a new major version to be released package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
9 participants
@Jessidhia @armano2 @nickserv @mysticatea @JoshuaKGoldberg @gavinbarron @bradzacher @j-f1 @7heQuietOne