-
Notifications
You must be signed in to change notification settings - Fork 79
chore: turn some javascript and typescript lint rules back into errors MONGOSH-1508 #1631
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
Conversation
if (${JSON.stringify(flags ?? '')}) { | ||
assert.deepStrictEqual((${JSON.stringify( | ||
flags | ||
// eslint-disable-next-line no-useless-escape |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I this case I think the no-useless-escape rule was a false positive. It doesn't seem to know about \s
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s because this whole section is inside a template string, so the actual regex ends up being /s+/
, which is of course actually wrong.
This might be another good use case for String.raw
? Either way, not a false positive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aah I see now. OMG that code is hard to read.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few very minor questions and small suggestions that can be ignored, looks good otherwise!
packages/node-runtime-worker-thread/src/spawn-child-from-source.ts
Outdated
Show resolved
Hide resolved
if (${JSON.stringify(flags ?? '')}) { | ||
assert.deepStrictEqual((${JSON.stringify( | ||
flags | ||
// eslint-disable-next-line no-useless-escape |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s because this whole section is inside a template string, so the actual regex ends up being /s+/
, which is of course actually wrong.
This might be another good use case for String.raw
? Either way, not a false positive
|
||
bus.on('mongosh:connect', function (args: ConnectEvent) { | ||
const connectionUri = redactURICredentials(args.uri); | ||
// eslint-disable-next-line @typescript-eslint/no-unused-vars |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really wish these rules would understand underscore-prefixed names or at least some kind of pattern for avoiding false positives 🙂
if ( | ||
settings !== null && | ||
Object.prototype.hasOwnProperty.call(settings, 'activeWindow') | ||
) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, if you prefer not to use String.raw
here I can do that in a follow-up PR
I'm leaving or ticketing the rest of the eslint rules that we overrode to be warnings rather than errors.