-
Notifications
You must be signed in to change notification settings - Fork 2
Add option to ignore single line comments #24
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
@@ -1,7 +1,7 @@ | |||
const cp = require('child_process') | |||
path = require('path'); | |||
|
|||
module.exports = (details, publish) => { | |||
module.exports.checkOwnership = (details, publish) => { |
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.
What is the reason for using a named export here instead of the default like it was using before? Doesn't seem to add any value?
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 don't know any other ways to make it mockable.
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 some digging and thinking and decided this will work
}, {}); | ||
|
||
Object.keys(emails).forEach((email) => { | ||
// { size, ownedLines, author, file_path } | ||
checkOwnership({ | ||
size: lines, | ||
utils.checkOwnership({ |
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.
same comment about the named export vs the default. Also utils implies that there are more utils in the module other than checkOwnership.
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.
utils
is kinda just a random name, could be anything else really. The reason is the same as above.
No description provided.