homebridge-onkyo icon indicating copy to clipboard operation
homebridge-onkyo copied to clipboard

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

Open jondthompson opened this issue 3 years ago • 3 comments

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 avatar Dec 17 '20 22:12 jondthompson

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

cbrandlehner avatar Dec 18 '20 09:12 cbrandlehner

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.

jondthompson avatar Dec 21 '20 15:12 jondthompson

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 avatar Dec 23 '20 09:12 cbrandlehner