-
-
Notifications
You must be signed in to change notification settings - Fork 367
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
Handle lack of package.json in expiring-todo-comments
rule
#397
Conversation
expiring-todo-comments
rule
@@ -17,7 +17,8 @@ const MESSAGE_ID_ENGINE_MATCHES = 'engineMatches'; | |||
const MESSAGE_ID_REMOVE_WHITESPACES = 'removeWhitespaces'; | |||
const MESSAGE_ID_MISSING_AT_SYMBOL = 'missingAtSymbol'; | |||
|
|||
const pkg = readPkgUp.sync().package; | |||
const hasPackage = readPkgUp.sync(); | |||
const pkg = hasPackage ? hasPackage.package : {}; |
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.
This two line seems weird.
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.
first thinking
const {package: pkg = {}} = readPkgUp.sync() || {};
const hasPackage = ??? // can't detect
next might better, but extra line
const result= readPkgUp.sync();
const hasPackage = Boolean(result)
const pkg = hasPackage ? result.package : {};
or maybe this one
const {
package: pkg = {},
hasPackage = true
} = readPkgUp.sync() || {hasPackage: false}
@lubien what do you think
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 agree. I just needed to get this out.
Should probably have been:
const packageResult = readPkgUp.sync();
const hasPackage = Boolean(packageResult);
const packageJson = hasPackage ? packageResult.package : {};
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.
Did we comment at the same time? LOL
what do you think my last code
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.
Hehe. Seems so. I didn't see your comment until now. Let's go with mine, which is your second one, just with more verbose naming.
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.
/cc @lubien Could you please send another PR to apply this?
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.
Sure! But I need a few hours before I can do this if it's not an issue.
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.
'fixme', | ||
'xxx' | ||
], | ||
terms: ['todo', 'fixme', 'xxx'], |
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.
In the future, don't do unrelated changes.
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.
Oh. Might have been the linter but might bad for leaving it, sorry. Won't happen again.
@garyking came with this scenario of a packageless project. For this my choice would be not to allow package related triggers to work at least until a proper package.json file is set up.
Even tho the original issue says eslint 5, this happens also in version 6.
Fixes #394