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

tools: Add no useless regex char class rule #9591

Closed

Conversation

princejwesley
Copy link
Contributor

@princejwesley princejwesley commented Nov 13, 2016

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

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Nov 13, 2016
@Trott
Copy link
Member

Trott commented Nov 14, 2016

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

@Trott
Copy link
Member

Trott commented Nov 14, 2016

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
Copy link
Contributor Author

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

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

@Trott
Copy link
Member

Trott commented Nov 14, 2016

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.

Copy link
Contributor

@not-an-aardvark not-an-aardvark left a comment

Choose a reason for hiding this comment

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

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').

fix: (fixer) => {
const start = node.range[0] + startOffset;
return fixer.replaceText({
range: [start, start + 1]
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

if (char === ']' && state.inCharClass) {
const charClass = charList[charListIdx];
charClass.emtpy = charClass.chars.length === 0;
if (charClass.emtpy) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think this is supposed to say empty.

* ]
*/

function parseRegExpCharClass(regExpText) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

Copy link
Contributor

@not-an-aardvark not-an-aardvark left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

Trott commented Nov 16, 2016

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
Copy link
Member

Trott commented Nov 24, 2016

Landed in 0f4c7b8 and 5cf0157

@Trott Trott closed this Nov 24, 2016
Trott pushed a commit to Trott/io.js that referenced this pull request Nov 24, 2016
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 <[email protected]>
Trott pushed a commit to Trott/io.js that referenced this pull request Nov 24, 2016
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 <[email protected]>
addaleax pushed a commit that referenced this pull request Dec 5, 2016
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 <[email protected]>
addaleax pushed a commit that referenced this pull request Dec 5, 2016
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 <[email protected]>
@Fishrock123 Fishrock123 mentioned this pull request Dec 5, 2016
2 tasks
@MylesBorins
Copy link
Contributor

@Trott would you be willing to manually backport?

@Trott
Copy link
Member

Trott commented Dec 20, 2016

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

@MylesBorins
Copy link
Contributor

@princejwesley would you be willing to backport?

MylesBorins pushed a commit that referenced this pull request May 8, 2017
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 <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 23, 2017
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants