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

Unnecessary escape character? #704

Closed
dcousens opened this issue Nov 25, 2016 · 8 comments

Comments

@dcousens
Copy link
Member

commented Nov 25, 2016

Unnecessary escape character: \]. (no-useless-escape)

The regex:

/Expected property "1" of type \[Buffer\], got String "foobar"/

Is this one for eslint? Or am I misunderstanding?

edit: This apparently works... is it even sane though? It feels so odd.

/Expected property "1" of type \[Buffer], got String "foobar"/
@dcousens

This comment has been minimized.

Copy link
Member Author

commented Nov 25, 2016

This only started happening in a recent update.
(forcing me to do bitcoinjs/bitcoinjs-lib@579fcce)

@feross

This comment has been minimized.

Copy link
Member

commented Nov 25, 2016

The no-useless-escape rule got stricter in the latest version of eslint. I think they're right that it's technically useless to escape \] but I agree that this is a tad bit aggressive.

This might be the source of the issue: eslint/eslint#7424

@feross

This comment has been minimized.

Copy link
Member

commented Nov 25, 2016

One good argument for why ESLint should not warn about \] is that regex's "Unicode mode" (which @mathiasbynens recently described as "strict mode" for regex literals) doesn't warn about it.

/\a/u - throws because a is always an invalid escape sequence. Prevents user from making a mistake.
/\)/u - does not throw even though ) does not need to be escaped. This is because ) sometimes does need to be escaped, and it doesn't hurt to just allow it. Very unlikely this is a user mistake.

This let's users keep things nice and parallel in their regex, like: /\[Buffer\]/u. My opinion: if it's good enough for regex "Unicode mode", then it's good enough for ESLint. 👍

@feross

This comment has been minimized.

Copy link
Member

commented Nov 25, 2016

I opened an issue: eslint/eslint#7656

@dcousens

This comment has been minimized.

Copy link
Member Author

commented Nov 26, 2016

Thanks for championing this @feross

@feross feross added blocked and removed need more info labels Dec 5, 2016

@dcousens

This comment has been minimized.

Copy link
Member Author

commented Dec 17, 2016

Close in favour of eslint/eslint#7656?

@feross

This comment has been minimized.

Copy link
Member

commented Dec 18, 2016

Sure. If they end up adding a way to specify exceptions to this rule, we can open a new issue for that.

@feross feross closed this Dec 18, 2016

@feross

This comment has been minimized.

Copy link
Member

commented Jan 11, 2017

FYI, the original eslint issue was fixed: eslint/eslint#7656

@dcousens dcousens changed the title Unnecessary escape character apparently Unnecessary escape character? Jan 11, 2017

@lock lock bot locked as resolved and limited conversation to collaborators May 10, 2018

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