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

Updating eslint and ran into some regex errors that appear to not be, but want clarification. #88

Open
jondthompson opened this issue Dec 17, 2020 · 3 comments
Labels
question stale_due_to_inactivity Used for issues raised but requested feedback is missing

Comments

@jondthompson
Copy link

So I updated ESlint and xo, as my npm test said there were some minor security issues. Most of the fixes were straightforward and result in better code like two if statements in a row being turned into one with '&&', and add one being operator++ rather than o = o + 1.

However, there are three lines that are regex replaces. The first two deal with curly quotes in the data from the device (and I'm pretty sure I caught a bug in that the if statement is triggered on the same Curley bracket twice on line 198 and might be why "26" needs a special handler), and the third deals with some sort of garbage text in the event input.

Anyhow, they all three seem to be escaped out of the older version of eslint with a comment of "eslint-disable no-useless-escape". However, that isn't working in the newer eslint as it is triggered by unicorn/better-regex.

So it looks like these regex lines are intentional, but I can't for the life of me understand why. If they need to search for the escape, they should be escaping the escape (\). If they don't, they should be simplified.

Please let me know which should be done and I'll update my fork and send a pull request for the whole shebang.

@jondthompson jondthompson added the bug A CONFIRMED bug in the plugin code label Dec 17, 2020
@cbrandlehner
Copy link
Collaborator

You can also disable the check in this case. See package.json for other checks already disabled for similar reasons.

@jondthompson
Copy link
Author

I'm questioning the need to have the code that violates the check. The first pair is a regex that deals with right and left curly double quotes. (/\“/g and /\”/g, which don't need to be escaped at all according to the lint). The second is /[[]"]+/g, which is the equivalent to /[[]"]+/g and searches for [,], and ".

If it were browser code, I'd understand that there can be nuances and this might be necessary, but if possible I'd rather see the "correct" Regex and a comment explaining what and why it's doing the replace than code that may or may not work properly in the first place and in the future.

@cbrandlehner
Copy link
Collaborator

You are welcome to improve the code. :-)
I did not dare to touch that part of the code nor can I recall who wrote that part.

@cbrandlehner cbrandlehner added question stale_due_to_inactivity Used for issues raised but requested feedback is missing and removed bug A CONFIRMED bug in the plugin code labels Aug 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question stale_due_to_inactivity Used for issues raised but requested feedback is missing
Projects
None yet
Development

No branches or pull requests

2 participants