ESLint Plugin: Restrict tolerances as fixable error

[aduth]

Continues #13757

This pull request continues from #13757, eliminating tolerances as described in that pull request:

• Normalize /** and /*
• Case insensitive "Dependencies" vs. "dependencies"
• Ending period
• "Node" dependencies as an alias for External

Note: The tolerances still technically exist in the implementation, but are now used as part of the "fixer" behavior to identify the comment as eligible to have its value be substituted/upgrade using the corrected value.

(See example fixer output)

Testing instructions:

Ensure lint passes:

npm run lint-js

Optional: Introduce an error case and verify fixer behavior:

npm run lint-js:fix

[gziolo]

Code changes generated by the updated rule and its fixer look great.

I'm not familiar with the ESlint API so I might not be the best person to accept this PR. Let me do some more testing locally.

[gziolo]

There was probably some reason to use Node keyword but I'm happy to see it merged into External group 👍

[aduth]

There was probably some reason to use Node keyword but I'm happy to see it merged into External group 👍

I suppose the idea is the imports are from the Node.js API, not any explicit dependency. I could see there being some value in it, but it's not something we either codified or use consistently. Same goes for "Browser dependencies".

As part of this overall effort, I'm fine to make adjustments from what we've standardized. But the important thing is forcing ourselves to have these conversations, rather than let inconsistencies grow via the unwritten conventions of a few people.

[gziolo]

diff --git a/packages/block-library/src/audio/icon.js b/packages/block-library/src/audio/icon.js
index 4b871506b..46ea192be 100644
--- a/packages/block-library/src/audio/icon.js
+++ b/packages/block-library/src/audio/icon.js
@@ -1,5 +1,5 @@
 /**
- * WordPress dependencies
+ * Internal dependencies
  */
 import { Path, SVG } from '@wordpress/components';

I tried the above and noticed that running npm run lint-js -- --fix adds WordPress dependencies but doesn't remove Internal dependencies`. As a next step, we could add logic which also detects code comments which don't contain imports at all.

At the moment the following code passes linter checks:

/**
 * Internal dependencies
 */
/**
 * WordPress dependencies
 */
import { Path, SVG } from '@wordpress/components';

this is the output from the aforementioned call on the diff.

[gziolo]

This looks like a great improvement. I left my comments but I don't find them as blockers but rather a future improvement. Again, it would be great to have another 👀 on the the actual implementation :)

Oh, and there is now merge conflict to be resolved after the name of the test file has changed.

[oandregal]

I've tested that ESLint is indeed stricter to report the comment.

The fixer, though, is able to substitute comments with some tolerance in dependencies (whether it has a final dot or not, and whether the first letter is in uppercase or lowercase) but not in the locality. In any of these cases:

/**
 * Wordpress dependencies.
 */

/**
 * wordpress dependencies.
 */

/**
 * internal dependencies.
 */

/**
 * external dependencies.
 */

/**
 * node dependencies.
 */

what happens is that a new comment properly formatted is added, but the old is not removed so it ends up as:

/**
 * External dependencies
 */
/**
 * node dependencies.
 */

Can we add some tolerance to the locality as well as per the examples above? With those implemented, the fixer will be able to actually fix 99% of use cases.

[aduth]

@nosolosw I'm not sure I follow the specific issue. Could you fork the original snippet to demonstrate a full example of problematic code?

[oandregal]

@aduth sure, here it is: https://astexplorer.net/#/gist/4bd0d3532fdc5944fa75ce3913af72cb/a13b33187d0decf2014c7b5ae34b50b85ca5daa6

In that example, changing external dependencies by node dependencies will also fail, while using Node dependencies works as expected (will update my comment).

[aduth]

@nosolosw I think the root issue there is capitalization of the first word. A quick test of a substitution to make the pattern case-insensitive seems to work as expected:

-const pattern = new RegExp( `^\\*?\\n \\* ${ locality } [dD]ependencies\\.?\\n $` );
+const pattern = new RegExp( `^\\*?\\n \\* ${ locality } dependencies\\.?\\n $`, 'i' );

https://astexplorer.net/#/gist/cbc01be66b166e0481caf4a966bcf6ae/b365527301f5a0254612ca6059580d712a089636