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

Improve usage of eslint-disable directives #16941

Merged
merged 7 commits into from
Aug 9, 2019

Conversation

swissspidy
Copy link
Member

Description

This PR attempts to improve the way eslint-disable is being used throughout the project.

In a first run npm run lint-js -- --report-unused-disable-directives has been used to identify and remove eslint-disable directives that were unused and thus unnecessary.

In a second run, eslint-plugin-eslint-comments has been installed and configured that does the same thing, and much more. For example, it points out ESLint rules that have been disabled but not re-enabled again, or rules that have been enabled but were not actually disabled in the first place.

How has this been tested?

ESLint passes just fine after these changes.

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

Verified by running `npm run lint-js -- --report-unused-disable-directives` before and after making these changes.
@gziolo
Copy link
Member

gziolo commented Aug 8, 2019

I see you had some issues with the lock file. I added docs with what worked for me when I did some extensive testing this week:

https://github.com/WordPress/gutenberg/tree/master/packages#managing-dependencies

@dsifford suggests manual updates of package.json files and running lerna bootstrap from the root folder. I haven't tried but I plan to as it seems slightly simpler.

@ntwb
Copy link
Member

ntwb commented Aug 8, 2019

Looking good to me, will compliment #16795 nicely 👌🏼

@swissspidy swissspidy added [Type] Enhancement A suggestion for improvement. [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. labels Aug 8, 2019
webpack.config.js Show resolved Hide resolved
packages/block-library/src/image/edit.js Outdated Show resolved Hide resolved
packages/components/src/sandbox/index.js Show resolved Hide resolved
@@ -21,6 +21,7 @@ module.exports = {
extends: [
'plugin:@wordpress/eslint-plugin/recommended',
'plugin:jest/recommended',
'plugin:eslint-comments/recommended',
Copy link
Member

Choose a reason for hiding this comment

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

Does order of those configs matter?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not that I am aware of. Can't find anything in the docs that would indicate this.

Copy link
Contributor

@dsifford dsifford Aug 8, 2019

Choose a reason for hiding this comment

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

I'm not certain here but I want to say that order does matter. Because what if 2 plugins set configuration for a single rule in different ways?

Edit: It appears to derive the final configuration in order from first to last, as described here: (https://eslint.org/docs/user-guide/configuring#extending-configuration-files). It doesn't explicitly state this though.

Copy link
Member

Choose a reason for hiding this comment

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

I ran a quick test eslint --print-config .eslintrc.js

https://gist.github.com/ntwb/d04000c8a29bd441c05b20650c027ed8/revisions

The order of the rules is changed, but, I agree that there is a chance of conflicting rules having some unknown behavior.

Copy link
Member

Choose a reason for hiding this comment

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

It looks like we are good here with the current order. It's a bit surprising though that rules are applied this way. I assumed the opposite.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Nice work, this should help us keep the codebase cleaner 💯

@@ -21,6 +21,7 @@ module.exports = {
extends: [
'plugin:@wordpress/eslint-plugin/recommended',
'plugin:jest/recommended',
'plugin:eslint-comments/recommended',
Copy link
Member

Choose a reason for hiding this comment

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

It looks like we are good here with the current order. It's a bit surprising though that rules are applied this way. I assumed the opposite.

@swissspidy swissspidy merged commit 853b3cb into master Aug 9, 2019
@swissspidy swissspidy deleted the fix/eslint-disable-directives branch August 9, 2019 11:25
@youknowriad youknowriad added this to the Gutenberg 6.3 milestone Aug 9, 2019
gziolo pushed a commit that referenced this pull request Aug 29, 2019
* Remove unused `eslint-disable` directives
* Leverage `eslint-plugin-eslint-comments` and fix reported errors
gziolo pushed a commit that referenced this pull request Aug 29, 2019
* Remove unused `eslint-disable` directives
* Leverage `eslint-plugin-eslint-comments` and fix reported errors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants