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 eslint-plugin-eslint-plugin #818

Merged
merged 1 commit into from Oct 28, 2021

Conversation

bmish
Copy link
Contributor

@bmish bmish commented Oct 24, 2021

  1. Enforces the recommended practices for eslint rules and rule tests using eslint-plugin-eslint-plugin which is commonly used internally by eslint plugins.
  2. Disables eslint-plugin/require-meta-type which can be addressed as a follow-up.
  3. Exports rules using export default instead of module.exports =. Why?
    • This is needed so that eslint-plugin-eslint-plugin will recognize the rules since export default is expected in files that use sourceType: module.
    • For module files (which import statements instead of require), we might as well use export statements instead of module.exports too to adopt fully modern syntax.

@ljharb
Copy link
Member

ljharb commented Oct 24, 2021

There’s no need to make eslintrc valid json; trailing commas and comments are a good feature.

scripts/boilerplate/doc.js Outdated Show resolved Hide resolved
src/rules/alt-text.js Outdated Show resolved Hide resolved
@bmish
Copy link
Contributor Author

bmish commented Oct 24, 2021

There’s no need to make eslintrc valid json; trailing commas and comments are a good feature.

I agree those are good features. It's just a bit inconvenient to use those in JSON files since some IDEs will show syntax warnings.

It's also confusing that the .eslintrc in this project has a mixture of single quotes, double quotes, and no quotes surrounding property names, so it's very difficult to tell what style to follow.

That's why I normally use the JS format of the config file .eslintrc.js so I can just let prettier handle this for me.

Anyways, I reverted the changes to the original lines in .eslintrc.

@codecov
Copy link

codecov bot commented Oct 24, 2021

Codecov Report

Merging #818 (94a4684) into master (f44fc05) will not change coverage.
The diff coverage is n/a.

❗ Current head 94a4684 differs from pull request most recent head f1414cf. Consider uploading reports for the commit f1414cf to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master     #818   +/-   ##
=======================================
  Coverage   99.22%   99.22%           
=======================================
  Files          98       98           
  Lines        1417     1417           
  Branches      477      477           
=======================================
  Hits         1406     1406           
  Misses         11       11           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f44fc05...f1414cf. Read the comment docs.

@ljharb ljharb force-pushed the eslint-plugin-eslint-plugin branch from 3c711de to 813b58e Compare October 28, 2021 14:14
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.

Thanks!

@ljharb ljharb force-pushed the eslint-plugin-eslint-plugin branch from 94a4684 to f1414cf Compare October 28, 2021 14:22
@ljharb ljharb merged commit f1414cf into jsx-eslint:master Oct 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants