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

Add config notice to rule docs #3362

Merged
merged 1 commit into from
Aug 23, 2022
Merged

Conversation

bmish
Copy link
Contributor

@bmish bmish commented Aug 19, 2022

This PR adds the following rule doc notice specifying what configs each rule belongs to.

  • 💼 This rule is enabled in the following configs: all, recommended.

For a few rules, the notice will also indicate when the rules are disabled by a particular config.

This improves usability by enabling users to clearly and easily find out if a rule is applying to their codebase given their configuration.

This notice is tested to ensure it isn't forgotten.

It's common for ESLint and many popular ESLint plugins to include a notice like this too. A couple examples:

@codecov
Copy link

codecov bot commented Aug 19, 2022

Codecov Report

Merging #3362 (036927c) into master (6207a04) will not change coverage.
The diff coverage is n/a.

❗ Current head 036927c differs from pull request most recent head 1656707. Consider uploading reports for the commit 1656707 to get more accurate results

@@           Coverage Diff           @@
##           master    #3362   +/-   ##
=======================================
  Coverage   97.54%   97.54%           
=======================================
  Files         123      123           
  Lines        8873     8873           
  Branches     3244     3244           
=======================================
  Hits         8655     8655           
  Misses        218      218           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

docs/rules/display-name.md Outdated Show resolved Hide resolved
docs/rules/jsx-space-before-closing.md Outdated Show resolved Hide resolved
@ljharb
Copy link
Member

ljharb commented Aug 21, 2022

I don't think this needs to be closed - I'm not saying we can't indicate it somewhere, but I don't want it prominently displayed.

@ljharb ljharb reopened this Aug 21, 2022
@bmish
Copy link
Contributor Author

bmish commented Aug 21, 2022

I don't think this needs to be closed - I'm not saying we can't indicate it somewhere, but I don't want it prominently displayed.

Hmm, well how else would we indicate it other than with the new standardized rule doc notice system we have?

@bmish bmish changed the title Add recommended or deprecated notices to rule docs Add recommended notice to rule docs Aug 21, 2022
@bmish bmish force-pushed the docs-rule-recommended branch 2 times, most recently from 0307999 to 3a65a5c Compare August 21, 2022 18:56
@ljharb
Copy link
Member

ljharb commented Aug 21, 2022

What if we listed all the configs a rule was in, with a comma-separated list and no icon?

@bmish
Copy link
Contributor Author

bmish commented Aug 21, 2022

What if we listed all the configs a rule was in, with a comma-separated list and no icon?

Okay, presumably there will only ever be recommended and all configs? So it would look like this for recommended rules:

💼 This rule is part of the following [configs](https://github.com/jsx-eslint/eslint-plugin-react#shareable-configurations) configs: `all`, `recommended`

or this for non-recommended rules:

💼 This rule is part of the following [configs](https://github.com/jsx-eslint/eslint-plugin-react#shareable-configurations) configs: `all`

I included a generic "config" emoji so that the notice lines up nicely with the the other fixable/suggestable notices that may be present and looks like an actual notice as opposed to just the rule detail body.

If this sounds good, I'll update the PR.

@ljharb
Copy link
Member

ljharb commented Aug 21, 2022

There's also jsx-runtime (so it'd be good to import the index file and dynamically use everything in the configs object). and yeah, that looks good.

@bmish bmish changed the title Add recommended notice to rule docs Add config notice to rule docs Aug 23, 2022
@ljharb
Copy link
Member

ljharb commented Aug 23, 2022

hm, i don't see anything listing jsx-runtime, but there should be two rules doing so.

@bmish
Copy link
Contributor Author

bmish commented Aug 23, 2022

@ljharb I've updated to the new notice style. It looks like there are no rules in the jsx-runtime config. The two rules specified there are actually configured to be off using 0.

https://eslint.org/docs/latest/user-guide/configuring/rules#configuring-rules

@ljharb
Copy link
Member

ljharb commented Aug 23, 2022

oh right, hmm - that still seems important to list, though, since including a config with a rule set to off will overwrite any previous config enabling them?

@bmish
Copy link
Contributor Author

bmish commented Aug 23, 2022

oh right, hmm - that still seems important to list, though, since including a config with a rule set to off will overwrite any previous config enabling them?

I can update the notice to mention which configs turn the rule off too.

@bmish bmish force-pushed the docs-rule-recommended branch 2 times, most recently from 237796d to 2014782 Compare August 23, 2022 04:17
@bmish
Copy link
Contributor Author

bmish commented Aug 23, 2022

@ljharb the notice now mentions when a rule is turned off by a config.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

tests/index.js Outdated Show resolved Hide resolved
tests/index.js Outdated Show resolved Hide resolved
markdown.config.js Outdated Show resolved Hide resolved
tests/index.js Outdated Show resolved Hide resolved
@ljharb ljharb merged commit 1656707 into jsx-eslint:master Aug 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants