tools: Add no useless regex char class rule

[princejwesley]

Checklist

• make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
• commit message follows commit guidelines

Affected core subsystem(s)

tools

Description of change

Eslint Rule:
Disallow useless escape in regex character class
with optional override characters option and auto
fixable with eslint --fix option.

Usage:
no-useless-regex-char-class-escape: [2, { override: ['[', ']'] }]

Note : ~~~lint errors(10) due to this rule is not addressed. Reason is, override list is not finalised.~~~

CC: @Trott

[Trott]

Nice! Any plans to submit the override parameter option to ESLint core for the no-useless-escape rule?

[Trott]

Looking at the 10 things that this flags, I'd be OK with all of them getting fixed. :-D

Would love to see the option upstreamed to ESLint itself so as to make this rule unnecessary, but for now, this seems like a good approach to me.

Would be interested in @not-an-aardvark's opinion.

[princejwesley]

CI: https://ci.nodejs.org/job/node-test-pull-request/4837/

@Trott I am interested in @not-an-aardvark's opinion 😄

[Trott]

I suspect you know this, but just to be safe: When landing this, the commit that fixes the useless escapes should land before the commit the enables the linting rule. This way, there's no chance of someone doing a git bisect finding a broken commit by ending up on the commit that enables the rule but doesn't have things fixed yet.

[not-an-aardvark]

LGTM with a few nits.

However, I noticed that this doesn't check string literals. Assuming we're planning to turn off no-useless-escape in favor of this rule, we will no longer be enforcing against useless escapes in string literals (e.g. 'foo \ bar').

[not-an-aardvark]

Nit: This can be replaced with fixer.replaceTextRange([start, start + 1], '')

[princejwesley]

Done

[not-an-aardvark]

Nit: I think this is supposed to say empty.

[not-an-aardvark]

I noticed that this function is very different from the corresponding one in no-useless-escape. This isn't necessarily a problem, but is there a reason to change it?

[princejwesley]

it covers only RegExp character class.
(Possible) Useless Escape>(Possible) Useless RegExp Escape>Useless RegExp Character Class Escape (optional override)

[princejwesley]

cI: https://ci.nodejs.org/job/node-test-pull-request/4838/

[not-an-aardvark]

The implementation LGTM. I am still a bit worried about what I mentioned here, though; if we disable the regular no-useless-escape rule, we won't be checking for useless escapes in strings.

[Trott]

if we disable the regular no-useless-escape rule, we won't be checking for useless escapes in strings.

I had imagined going this route:

• leave no-useless-escape enabled. It's missing all the things this new rule will find until we upgrade to ESLint 3.10.
• use this rule to fix character classes that we have and prevent new ones from being introduced

Then, at some point, we do one of the following. Either:

• Upgrade to ESLint 3.10.x or later and remove this custom rule (and deal with the handful of extra errors--either disable linting on a line-by-line basis or give up on the readability-exception dream and "fix" those remaining errors).

...or...

• Wait for (or contribute) an exception mechanism to be introduced into the no-useless-escape rule so that we can specify the exceptions for no-useless-escape in .eslintrc and then disable this custom rule.

[Trott]

Landed in 0f4c7b8 and 5cf0157

[MylesBorins]

@Trott would you be willing to manually backport?

[Trott]

@thealphanerd Did you mean to @-mention @princejwesley instead?

[MylesBorins]

@princejwesley would you be willing to backport?