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

ESLint Plugin: Fix rule selectors #15839

Merged
merged 4 commits into from May 29, 2019

Conversation

@tfrommen
Copy link
Member

commented May 27, 2019

Description

The two (regular expression) selectors of the custom no-restricted-syntax rule in eslint-plugin are broken.

For example, the first regular expression, ^__|_n|_x$, is equivalent to the following list of patterns:

  • ^__
  • _n
  • _x$

Obviously, what the author had in mind, however, is this list:

  • ^__$
  • ^_n$
  • ^_x$

Because of the broken rule, my call to __experimentalGetSettings (imported from wp.date/@wordpress/date) got flagged; it matches the ^__ pattern/option.

Also, calling the fictional function foo_x() would also flag up, as it matches the last pattern/option, _x$.

In addition to the above bugs, the two selectors shuold also account for _nx, which is currently only tested for the third (i.e., index 2) argument, which is yet another error. It should be checked for indexes 0, 1 and 3, that is: single, plural and context (not number).

How has this been tested?

Run ESLint with the old and with the new rule on this code:

const { __experimentalGetSettings } = wp.date;
const { __, _n, _nx, _x } = wp.i18n;

const foo_x = () => {};

const foo = 'bar';

/**
 * Should NOT flag
 */
__( 'Some text', 'text-domain' );
_n( 'Some text', 'Some texts', 42, 'text-domain' );
_nx( 'Some text', 'Some texts', 42, 'Some context', 'text-domain' );
_x( 'Some text', 'Some context', 'text-domain' );
const settings = __experimentalGetSettings();
foo_x();

/**
 * SHOULD flag
 */
__( foo, 'text-domain' );
_n( foo, 'Some texts', 42, 'text-domain' );
_nx( foo, 'Some texts', 42, 'Some context', 'text-domain' );
_x( foo, 'Some context', 'text-domain' );

_n( 'Some text', foo, 42, 'text-domain' );
_nx( 'Some texts', foo, 42, 'Some context', 'text-domain' );
_x( 'Some text', foo, 'text-domain' );

_nx( 'Some text', 'Some texts', 42, foo, 'text-domain' );

Types of changes

Fix several issues with ESLint rule selectors. Most of them have to do with the regular expressions, one issue is an incorrect argument index.

By the way, I decided to go with _nx as new sub-pattern/option, and not make _n be _nx? or so. Both for cinsistency and better readability.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

tfrommen added some commits May 27, 2019

@tfrommen

This comment has been minimized.

Copy link
Member Author

commented May 27, 2019

Ideally, the rule no-restricted-syntax would be moved from the config file to a separate file, which then should have a test file. Thoughts?

@gziolo gziolo requested a review from swissspidy May 27, 2019

@gziolo

This comment has been minimized.

Copy link
Member

commented May 27, 2019

Ideally, the rule no-restricted-syntax would be moved from the config file to a separate file, which then should have a test file. Thoughts?

I guess it is never executed inside Gutenberg as we override no-restricted-syntax... I agree that this could be extracted to its own rule to fix this issue at the same time.

@ntwb

ntwb approved these changes May 27, 2019

Copy link
Member

left a comment

LGTM 👍

Added the changelog entry in 8e93ae3

@aduth

aduth approved these changes May 28, 2019

Copy link
Member

left a comment

Whoops, I'll take the blame on this one 😄

Nice fix 👍

Since there were technically two changes of this pull request, I added another CHANGELOG entry in d3cc463 to account for the _nx arguments placement fix.

@gziolo

This comment has been minimized.

Copy link
Member

commented May 28, 2019

Should we copy those rules to

'no-restricted-syntax': [
?

@aduth

This comment has been minimized.

Copy link
Member

commented May 28, 2019

@gziolo Oh, that's a good point re: these rules not actually taking effect. As the minimal effort short-term fix, that seems like the best option. I don't have any preference to whether that occurs here, since it's arguably a "separate" fix. I guess it would be fine enough to tackle here.

@gziolo gziolo added this to the 5.9 (Gutenberg) milestone May 29, 2019

@gziolo gziolo merged commit a606c4b into master May 29, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@gziolo gziolo deleted the fix-eslint-rule-selectors branch May 29, 2019

@tfrommen

This comment has been minimized.

Copy link
Member Author

commented May 29, 2019

Oh well. I just pushed a new commit that now just vanished into Nirvana...

Will do that in another PR then... 😞

@gziolo

This comment has been minimized.

Copy link
Member

commented May 29, 2019

I was about to open new PR to handle it, feel free to do it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.