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.

Sign up for GitHub

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

Jump to bottom

feat(rule-finder): Omit deprecated rules #270

Merged
merged 3 commits into from
Jun 13, 2017
Merged

feat(rule-finder): Omit deprecated rules #270

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0dc7434
feat(rule-finder): Omit deprecated rules
randycoulman May 19, 2017
7b9df81
feat(rule-finder): Remove default parameter value from RuleFinder
randycoulman Jun 13, 2017
0931677
feat(rule-finder): Switch up where the default options are specified
randycoulman Jun 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/lib/rule-finder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ function _isNotCore(rule) {
return rule.indexOf('/') !== '-1';
}

function RuleFinder(specifiedFile, options) {
function RuleFinder(specifiedFile, options = {}) {
const {omitCore, includeDeprecated} = options;
const configFile = _getConfigFile(specifiedFile);
const config = _getConfig(configFile);
Expand Down Expand Up @@ -108,6 +108,6 @@ function RuleFinder(specifiedFile, options) {
this.getUnusedRules = () => getSortedRules(unusedRules);
}

module.exports = function (specifiedFile, options = {}) {
module.exports = function (specifiedFile, options) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please continue specifying the default here also; it avoids contributing to the functions length.

For coverage, are you calling the function in tests without an options arg, so as to cover the defaulting?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ljharb This is well-covered in the tests; the problem is that specifying the default in both places means that the one on the RuleFinder constructor isn't covered by tests because it's only ever called through this exported function.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should be specified in the exported place then; since that's what consumers see.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have thought exactly the other way around, the reason being the exported function only "dispatching" its arguments through, to the original RuleFinder. And exactly that is the place where errors would occur, when no option parameter would be specified.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ta2edchimp Can you just revert my last commit, or do you need me to submit a new PR with this change?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot simply push to this repo after using the git scalpel locally. So I would have reverted this with another separate commit when bumping the version number.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what git revert does, isn't it? My understanding is that it makes a new commit that backs out the changes that were made in the commit being reverted.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. I have no idea what I was thinking about.
Feel free to open a new PR regarding this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. #276

return new RuleFinder(specifiedFile, options);
};