Skip to content
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.

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

Remove meta.docs.category from core rules #13398

Closed
anikethsaha opened this issue Jun 6, 2020 · 21 comments · Fixed by #14594
Closed

Remove meta.docs.category from core rules #13398

anikethsaha opened this issue Jun 6, 2020 · 21 comments · Fixed by #14594
Assignees
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion breaking This change is backwards-incompatible documentation Relates to ESLint's documentation rule Relates to ESLint's core rules

Comments

@anikethsaha
Copy link
Member

anikethsaha commented Jun 6, 2020

The version of ESLint you are using.
master, 7.2.0

The problem you want to solve.
#13382
as discussed, there seems to be some inconsistency among rules that can either belong to es6 category or other categories.
Also as there are other rules as well that support es6+ specs like rest-spread-spacing and others, so I guess it might lead to categories these rules according to their es version.

Your take on the correct solution to problem.
So I guess it would be better to have no category based on es version

Should these are the following ecmascript-6 category rules that should change their category to following new categories

  • S : Stylistic Issues
  • P : Possible Errors
  • B : Best Practices
rule name new category
arrow-body-style B
arrow-parens B
arrow-spacing S
constructor-super P
generator-star-spacing S
no-class-assign P
no-confusing-arrow B
no-const-assign P
no-dupe-class-members P
no-duplicate-imports P
no-new-symbol P
no-restricted-exports not sure, may be P ?
no-restricted-imports same as 🔼 ?
no-this-before-super P
no-useless-computed-key B
no-useless-constructor B
no-useless-rename B
no-var B
object-shorthand B
prefer-arrow-callback P
prefer-const B
prefer-destructuring B
prefer-numeric-literals not sure
prefer-rest-params B
prefer-spread B
prefer-template B
require-yield B
rest-spread-spacing B
sort-imports S or B
symbol-description B
template-curly-spacing S
yield-star-spacing S

thoughts ?

Are you willing to submit a pull request to implement this change?
Yes

PS: I guess I picked the wrong template, it should have been rule changes, to core label should be replaced with rule ?

@anikethsaha anikethsaha added core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint triage An ESLint team member will look at this issue soon labels Jun 6, 2020
@lydell
Copy link

lydell commented Jun 6, 2020

Previous discussion: #7991

@aladdin-add aladdin-add added documentation Relates to ESLint's documentation evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Jun 13, 2020
@anikethsaha
Copy link
Member Author

anikethsaha commented Jun 14, 2020

Along with documentation changes, this would need changes in rules not in core. so I guess the core label can be removed.

@kaicataldo
Copy link
Member

kaicataldo commented Jun 25, 2020

Along with documentation changes, this would need changes in rules not in core. so I guess the core label can be removed.

Can you explain what you mean by this?

@kaicataldo
Copy link
Member

kaicataldo commented Jun 25, 2020

I agree that the ES6 category isn't particularly useful at this point and would love to see those rules categorized by more descriptive categories.

@anikethsaha
Copy link
Member Author

anikethsaha commented Jun 25, 2020

Along with documentation changes, this would need changes in rules not in core. so I guess the core label can be removed.

Can you explain what you mean by this?

currently, this issue is labeled with core which I think can be removed as this might not require to change anything in the core. Just rule's category changes and the docs.

May be some tests might be changed.

@kaicataldo
Copy link
Member

kaicataldo commented Jun 25, 2020

Oh, I see - that makes sense!

@kaicataldo kaicataldo added rule Relates to ESLint's core rules and removed core Relates to ESLint's core APIs and features labels Jun 25, 2020
@ljharb
Copy link
Sponsor Contributor

ljharb commented Jun 25, 2020

A rule's category is part of its observable metadata, though, no?

@kaicataldo
Copy link
Member

kaicataldo commented Jun 25, 2020

@ljharb Are you asking if this should be a breaking change or not? If so, I agree that we should publish this in a major release.

@anikethsaha
Copy link
Member Author

anikethsaha commented Jun 25, 2020

Yeah, this would require changes in the init command I guess, so I think it can be breaking as that may lead to different config generation.

@ljharb
Copy link
Sponsor Contributor

ljharb commented Jun 25, 2020

It would also break any tools in the ecosystem that read the rule's category.

@mdjermanovic mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion breaking This change is backwards-incompatible and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion enhancement This change enhances an existing feature of ESLint labels Jul 16, 2020
@anikethsaha
Copy link
Member Author

anikethsaha commented Jul 16, 2020

There are some not sure and or in the table above in the issue description.

Would appreciate some inputs/thoughts on them.

@nzakas
Copy link
Member

nzakas commented Jul 16, 2020

I’d recommend a slight change and suggest we use the rule type instead of categories. The rule types are errors, suggestions, and layout: https://eslint.org/docs/user-guide/command-line-interface#fix-type

We can pretty up the headings as Possible Errors, Suggestions, and Code Spacing, or whatever else makes sense. I’d just like us to stick with the three types we already have for simplicity.

@mdjermanovic
Copy link
Member

mdjermanovic commented Jul 16, 2020

I think it might be good to have more than three categories.

For example, no-with and no-continue are both type: "suggestion", but there should be a difference because one enforces a best practice, while the other is only stylistic.

@nzakas
Copy link
Member

nzakas commented Jul 17, 2020

@mdjermanovic Im not sure that making that distinction is important at this point in ESLint’s lifecycle. I’d like to get away from the term “best practices,” as well, so we can stay a little more neutral. We already have “eslint:recommended” to signify what we believe are the most important rules across the board.

Plus, if we stick with using the rule type on the page, this is no longer a breaking change because the meta data isn’t changing. Also, there’s a nice correlation with the use of —fix-type.

@nvmnghia
Copy link

nvmnghia commented Jul 18, 2020

Can someone explain why prefer-destructuring is a Best practice? I see that it works for multiple variables, like:

const { a, b, c, d } = object;

But this one is illegible:

const { a } = object;

Can a linter detect that there's just one variable, and don't enforce the rule?

@anikethsaha
Copy link
Member Author

anikethsaha commented Jul 18, 2020

IMO, prefer-destructuring is the best practice because,

  • Less code
  • more code readability.
  • works well for defaults (code quantity is quite less when destructuring)

and about the code you mentioned,
the object should have a property a otherwise it would be undefined

eg
image
image

You can refer the docs or this article for more details

@btmills btmills added this to Need Discussion in v8.0.0 Apr 8, 2021
@nzakas nzakas moved this from Need Discussion to Planned in v8.0.0 Apr 8, 2021
@btmills
Copy link
Member

btmills commented Apr 12, 2021

As part of this change for ESLint v8.0.0, we also want to remove the category key from core rules so it’s clear that users shouldn’t be relying on it.

@mdjermanovic mdjermanovic changed the title Remove ecmascript 6 category for rules Remove meta.docs.category from core rules Apr 12, 2021
@nzakas nzakas added this to Needs Triage in Triage via automation Apr 13, 2021
@nzakas nzakas moved this from Needs Triage to Ready to Implement in Triage Apr 13, 2021
aladdin-add added a commit to eslint/archive-website that referenced this issue May 17, 2021
aladdin-add added a commit to aladdin-add/eslint that referenced this issue May 17, 2021
chore: rm meta.cat from core rules

chore: do not require meta.cat in internal rules

chore: rename cat => ruletype
@aladdin-add aladdin-add moved this from Planned to Pull Request Opened in v8.0.0 May 17, 2021
aladdin-add added a commit to eslint/archive-website that referenced this issue May 18, 2021
aladdin-add added a commit to eslint/archive-website that referenced this issue May 18, 2021
@nzakas nzakas moved this from Ready to Implement to Pull Request Opened in Triage May 29, 2021
aladdin-add added a commit to eslint/archive-website that referenced this issue Jun 29, 2021
@nzakas nzakas moved this from Pull Request Opened to Ready for Merge in v8.0.0 Jul 1, 2021
aladdin-add added a commit to aladdin-add/eslint that referenced this issue Aug 6, 2021
chore: rm meta.cat from core rules

chore: do not require meta.cat in internal rules

chore: rename cat => ruletype

Chore: rename conf/category-list.json => rule-type-list.json

chore: update tools/update-rule-types

chore: review suggestions

Update conf/rule-type-list.json

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

Update Makefile.js

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Triage automation moved this from Pull Request Opened to Complete Aug 24, 2021
v8.0.0 automation moved this from Ready for Merge to Done Aug 24, 2021
mdjermanovic added a commit that referenced this issue Aug 24, 2021
)

* Breaking: remove category in core rules (fixes #13398)

chore: rm meta.cat from core rules

chore: do not require meta.cat in internal rules

chore: rename cat => ruletype

Chore: rename conf/category-list.json => rule-type-list.json

chore: update tools/update-rule-types

chore: review suggestions

Update conf/rule-type-list.json

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

Update Makefile.js

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

* chore: remove unused files

* fix: conflicts

* remove missingMetaDocsDescription message

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Bryan Mishkin <698306+bmish@users.noreply.github.com>
@ljharb
Copy link
Sponsor Contributor

ljharb commented Aug 24, 2021

This will probably create a lot of churn for the Airbnb style guide, since it provides entry points for each category, and groups config by category.

radomirbosak pushed a commit to sider/runners that referenced this issue Dec 23, 2021
The category field was removed from issue object as part of breaking changes in ESLint 8 .

See: eslint/eslint#13398
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Feb 21, 2022
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion breaking This change is backwards-incompatible documentation Relates to ESLint's documentation rule Relates to ESLint's core rules
Projects
Triage
Complete
v8.0.0
  
Done
Development

Successfully merging a pull request may close this issue.

9 participants