tools: Add no useless regex char class rule by princejwesley · Pull Request #9591 · nodejs/node

princejwesley · 2016-11-13T05:57:39Z

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 · 2016-11-14T04:40:49Z

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

Trott · 2016-11-14T04:43:30Z

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 · 2016-11-14T05:13:44Z

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

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

Trott · 2016-11-14T05:15:02Z

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 · 2016-11-14T05:47:50Z

tools/eslint-rules/no-useless-regex-char-class-escape.js

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

not-an-aardvark · 2016-11-14T06:20:09Z

tools/eslint-rules/no-useless-regex-char-class-escape.js

Nit: I think this is supposed to say empty.

not-an-aardvark · 2016-11-14T06:23:05Z

tools/eslint-rules/no-useless-regex-char-class-escape.js

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?

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

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: ['[', ']'] }] PR-URL: nodejs#9591

The `/` character does not need to be escaped when occurring inside a character class in a regular expression. Remove such instances of escaping in the code base. PR-URL: nodejs#9591

princejwesley · 2016-11-14T07:33:34Z

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 · 2016-11-16T21:29:39Z

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 · 2016-11-24T06:11:44Z

Landed in 0f4c7b8 and 5cf0157

The `/` character does not need to be escaped when occurring inside a character class in a regular expression. Remove such instances of escaping in the code base. PR-URL: nodejs#9591 Reviewed-By: Teddy Katz <teddy.katz@gmail.com>

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: ['[', ']'] }] PR-URL: nodejs#9591 Reviewed-By: Teddy Katz <teddy.katz@gmail.com>

The `/` character does not need to be escaped when occurring inside a character class in a regular expression. Remove such instances of escaping in the code base. PR-URL: #9591 Reviewed-By: Teddy Katz <teddy.katz@gmail.com>

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: ['[', ']'] }] PR-URL: #9591 Reviewed-By: Teddy Katz <teddy.katz@gmail.com>

MylesBorins · 2016-12-20T19:29:06Z

@Trott would you be willing to manually backport?

Trott · 2016-12-20T22:42:08Z

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

MylesBorins · 2016-12-21T19:21:35Z

@princejwesley would you be willing to backport?

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: ['[', ']'] }] PR-URL: #9591 Reviewed-By: Teddy Katz <teddy.katz@gmail.com>

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: ['[', ']'] }] PR-URL: nodejs/node#9591 Reviewed-By: Teddy Katz <teddy.katz@gmail.com>

nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Nov 13, 2016

not-an-aardvark suggested changes Nov 14, 2016

View reviewed changes

princejwesley added 2 commits November 14, 2016 12:52

tools: Add no useless regex char class rule

50b8434

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: ['[', ']'] }] PR-URL: nodejs#9591

lib,tools: remove unneeded escaping of /

1fd4f71

The `/` character does not need to be escaped when occurring inside a character class in a regular expression. Remove such instances of escaping in the code base. PR-URL: nodejs#9591

princejwesley force-pushed the tool-eslint-regex-char branch from dba2a78 to 1fd4f71 Compare November 14, 2016 07:29

not-an-aardvark approved these changes Nov 16, 2016

View reviewed changes

Trott closed this Nov 24, 2016

not-an-aardvark mentioned this pull request Nov 25, 2016

no-useless-escape is too aggressive eslint/eslint#7656

Closed

Fishrock123 mentioned this pull request Dec 5, 2016

v7.2.1 proposal #10127

Merged

2 tasks

MylesBorins added lts-watch-v4.x labels Dec 20, 2016

MylesBorins added backported-to-v6.x and removed lts-watch-v4.x labels May 8, 2017

MylesBorins mentioned this pull request May 23, 2017

v6.11.0 proposal #13059

Merged

Uh oh!

Conversation

princejwesley commented Nov 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

Trott commented Nov 14, 2016

Uh oh!

Trott commented Nov 14, 2016

Uh oh!

princejwesley commented Nov 14, 2016

Uh oh!

Trott commented Nov 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

not-an-aardvark left a comment

Choose a reason for hiding this comment

Uh oh!

not-an-aardvark Nov 14, 2016

Choose a reason for hiding this comment

Uh oh!

princejwesley Nov 14, 2016

Choose a reason for hiding this comment

Uh oh!

not-an-aardvark Nov 14, 2016

Choose a reason for hiding this comment

Uh oh!

not-an-aardvark Nov 14, 2016

Choose a reason for hiding this comment

Uh oh!

princejwesley Nov 14, 2016

Choose a reason for hiding this comment

Uh oh!

princejwesley commented Nov 14, 2016

Uh oh!

not-an-aardvark left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Nov 16, 2016

Uh oh!

Trott commented Nov 24, 2016

Uh oh!

MylesBorins commented Dec 20, 2016

Uh oh!

Trott commented Dec 20, 2016

Uh oh!

MylesBorins commented Dec 21, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

princejwesley commented Nov 13, 2016 •

edited

Loading

Trott commented Nov 14, 2016 •

edited

Loading