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

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

Merged
merged 3 commits into from Jun 13, 2017

Conversation

Projects
None yet
4 participants
@randycoulman
Copy link
Contributor

randycoulman commented May 19, 2017

Filter out deprecated rules in _getAllAvailableRules() and _getPluginRules(). With this change, the lists of rules displayed with the -a and -u flags will not include deprecated rules.

Breaking: The output is now different from what it was, as deprecated rules are now omitted by default.

Breaking: The getRuleFinder() API has changed. Previously, it took an optional filename and an optional boolean, noCore. Now, it takes the optional filename and an options object:

  • omitCore: Exclude core rules from the output
  • includeDeprecated: Include deprecated rules in the output

Both options default to false if they are not provided.

Per a review request, I've added a flag to allow deprecated rules to be included. Adding --include deprecated or -i deprecated will include deprecated rules in the output; they're not included by default.

To make the implementation of this flag cleaner, I refactored getRuleFinder() to take an options object instead of a boolean as the second argument. Adding a second boolean (includeDeprecated) made it a difficult interface to use.

Note that eslint.linter.getRules() returns an ES6 Map, which cannot be directly filtered. Also, its keys() function returns an iterable, so we have to spread that into an array before we can filter it. That's why the code is written the way it is.

To make this feature have a cleaner implementation, I also performed the following refactorings:

  • Don't pass plugin rules to _getAllAvailableRules. Plugin rules are now merged with core rules directly in RuleFinder instead.
  • Rename _getAllAvailableRules -> _getCoreRules to better reflect its purpose.

I also:

  • Removed the nc option; it isn't documented and isn't being used.

  • Split some of the find tests into two it blocks. I found that calling proxyquire twice in the same test wasn't doing anything, so was potentially hiding failures.

  • Renamed some rule-finder tests to result in more consistent output.

Fixes #172

@ljharb
Copy link
Collaborator

ljharb left a comment

I'm hesitant to make this breaking change; I think an option might be better.

return normalized.prefix + '/' + rule;
})
Object.keys(rules)
.filter(rule => !_isDeprecated(rules[rule]))

This comment has been minimized.

@ljharb

ljharb May 19, 2017

Collaborator

I would expect this line to be indented one level farther

This comment has been minimized.

@randycoulman

randycoulman May 19, 2017

Contributor

Yes, good catch. I think I was following the pattern of something else in the file. I'll fix that up.

})
Object.keys(rules)
.filter(rule => !_isDeprecated(rules[rule]))
.map(rule => normalized.prefix + '/' + rule)

This comment has been minimized.

@ljharb

ljharb May 19, 2017

Collaborator

This should use a template literal

This comment has been minimized.

@randycoulman

randycoulman May 19, 2017

Contributor

Agreed; I just moved the existing code around a bit. I'll take care of it.

];
function _getCoreRules() {
const rules = eslint.linter.getRules()
return [...rules.keys()]

This comment has been minimized.

@ljharb

ljharb May 19, 2017

Collaborator

Nbd but this would be slightly more efficient as Array.from(rules.keys(), rule => !_isDeprecated(rules.get(rule))).filter(Boolean)

This comment has been minimized.

@randycoulman

randycoulman May 19, 2017

Contributor

Ah, cool! That's way better. I didn't know about Array.from. I tend to use Ramda for stuff like this. I'll fix that up.

This comment has been minimized.

@randycoulman

randycoulman May 19, 2017

Contributor

Wait, no, that doesn't work. We need the actual keys out, and this is mapping them to Booleans, returning a list of true values.

This comment has been minimized.

@ljharb

ljharb May 19, 2017

Collaborator

Ah, good point. Yours is fine then :-)

This comment has been minimized.

@randycoulman

randycoulman May 19, 2017

Contributor

I switched it to Array.from instead - seems a bit clearer.

@randycoulman

This comment has been minimized.

Copy link
Contributor

randycoulman commented May 19, 2017

Regarding the "breaking-ness" of the change: IMO, this is more like the expected behavior and so should be the default. Also, we're already in breaking-change land due to #237, which this PR depends on. But if you really want this to be behind an option, I can do that.

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented May 19, 2017

Regardless of which is the default, we'd need an option for it - I'd want to be able to include deprecated rules.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented May 19, 2017

Codecov Report

Merging #270 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #270   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           9      9           
  Lines         179    186    +7     
=====================================
+ Hits          179    186    +7
Impacted Files Coverage Δ
src/lib/rule-finder.js 100% <100%> (ø) ⬆️
src/bin/find.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0cd85c8...0931677. Read the comment docs.

@randycoulman

This comment has been minimized.

Copy link
Contributor

randycoulman commented May 19, 2017

Code comments addressed; I'll take a look at adding the option later today (hopefully).

@ljharb
Copy link
Collaborator

ljharb left a comment

LGTM, pending the option.

To restate: I'm content with the default being "filter out the deprecated rules", making this PR semver-major, as long as there's an option to explicitly restore the behavior where deprecated rules are included.

@randycoulman

This comment has been minimized.

Copy link
Contributor

randycoulman commented May 19, 2017

@ljharb I could use some input on the flag to use for including/excluding deprecated rules; I'm trying to keep #188 in mind as well.

It feels like #188 should be another "option" (similar to -c, -a, etc), and I was thinking of naming it -d/--deprecated.

If that's what we do there, then I'm not sure what to use for the flag for this rule. Maybe --include-deprecated? Would that be too confusing? And what should the short version of the option be? -id?

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented May 19, 2017

I think --include=deprecated would be good, and then the short one could be -i deprecated?

@randycoulman

This comment has been minimized.

Copy link
Contributor

randycoulman commented May 21, 2017

I've added the --include deprecated/-i deprecated flag now. I did some additional refactoring as part of that; see the commit comments for details. I also updated the main PR description.

Let me know if you want me to remove the backwards-compatibility handling for the getRuleFinder() interface.

I see that the tests are failing on Travis; I'm not sure what's happening there. They're all passing locally.

README.md Outdated
@@ -42,15 +42,21 @@ The intended usage is as an npm script:
}
```

Then run it with: `$ npm run -s eslint-find-option-rules` (the `-s` is to silence npm output).

This comment has been minimized.

@ljharb

ljharb May 21, 2017

Collaborator

probably best to use --silent, rather than abbreviations - it's also more googleable.

This comment has been minimized.

@randycoulman

randycoulman May 21, 2017

Contributor

Fixed.

README.md Outdated

By default, core rules will be included in the output of `-c|--current`, `-a|--all-available`, and `-u|--unused`. If you want to report on plugin rules only, use the `--no-core` flag.

By default, deprecated rules will be omitted from the output of `-a|--all-available`, `-p|--plugin` and `-u|--unused`. If you want to report on deprecated rules as well, use the `--include deprecated` or `-i deprecated` flag.

This comment has been minimized.

@ljharb

ljharb May 21, 2017

Collaborator

not --include=deprecated?

This comment has been minimized.

@randycoulman

randycoulman May 21, 2017

Contributor

yargs works both ways. I updated the test and the doc to use the =, if only to have Travis try again and hopefully pass this time.

@ta2edchimp

This comment has been minimized.

Copy link
Collaborator

ta2edchimp commented May 21, 2017

Thanks so much for the work on this! Looks great and I'm looking forward to us finally supporting this long awaited and discussed feature.

Re: Failing tests on Travis
@randycoulman On what version of node did the tests pass for you locally? Travis runs against 6.10.3 and 7.10 ...

Re: Versioning and breaking changes
@ljharb So you would propose, we go "full semver", making the release including the PR this one depends on, as well as this one a major each?

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented May 21, 2017

@ta2edchimp we should always go full semver :-) that doesn't, however, mean that we need two majors - they can be bunched up as one.

@ta2edchimp

This comment has been minimized.

Copy link
Collaborator

ta2edchimp commented May 21, 2017

"full semver"... didn't know how to put it otherwise 😜
Of course, I'm with you to fully comply to semver. So, in the end, we get a new version with this cool feature! And it should then be a new major ...
I'm still puzzled as to why the Travis builds may fail and cannot look deeper into it until tomorrow, but I will try to then, and also try to get the time to release the previous one ( @ljharb I think you also have publish rights? So if you should have the time, feel free to npm publish it ).

@randycoulman

This comment has been minimized.

Copy link
Contributor

randycoulman commented May 21, 2017

@ta2edchimp I'm running 6.10.3 locally and having no problems with the tests. I just pushed up a new commit, so I'll see what happens.

@randycoulman

This comment has been minimized.

Copy link
Contributor

randycoulman commented May 21, 2017

The builds are still failing on Travis. From the failures, it looks like it's not picking up the changes I made to the RuleFinder constructor. It's acting as though it's treating the options object as a simple Boolean instead of as an object.

Perhaps I should try taking out the backwards-compatibility code to see what happens? Any other ideas?

@ta2edchimp

This comment has been minimized.

Copy link
Collaborator

ta2edchimp commented May 21, 2017

Perhaps I should try taking out the backwards-compatibility code to see what happens?

If you don't mind, please give that a try.
We have a breaking change anyways, so I see no problem in changing the argument to be required to be an object.

Nevertheless, I'm curious to find out what happens there.

@randycoulman

This comment has been minimized.

Copy link
Contributor

randycoulman commented May 21, 2017

OK, backwards-compatibility code removed. I also realized that I forgot to update the signature of the exported rule finder function; I was changing the constructor, but not the exported function. Maybe that will help on Travis?

@randycoulman

This comment has been minimized.

Copy link
Contributor

randycoulman commented May 21, 2017

I'm not sure what was going on, but I reversed the sense of the includeCore flag and renamed it to omitCore. That makes both flags consistently default to false, and also lets the build pass.

I think this is finally ready to merge. If you want me to squash commits and rebase on master, I'm happy to do that. Or, you can just squash when you merge.

@randycoulman

This comment has been minimized.

Copy link
Contributor

randycoulman commented May 21, 2017

PR description updated to make clear what the breaking changes are.

@ta2edchimp

This comment has been minimized.

Copy link
Collaborator

ta2edchimp commented May 21, 2017

Yes, please rebase on master. We can squash on merge.

@randycoulman randycoulman force-pushed the randycoulman:omit-deprecated-rules branch from 389e82e to d777afd May 21, 2017

@randycoulman

This comment has been minimized.

Copy link
Contributor

randycoulman commented May 21, 2017

I just squashed and rebased both. Should be ready to go now.

@randycoulman

This comment has been minimized.

Copy link
Contributor

randycoulman commented May 26, 2017

Is there anything else I need to do with this before it can be merged? I'd like to start working on #188 soon, and I'll need this merged in order to do that. Thanks!

@ljharb

ljharb approved these changes May 27, 2017

Copy link
Collaborator

ljharb left a comment

Seems legit. Thanks!

@ta2edchimp

This comment has been minimized.

Copy link
Collaborator

ta2edchimp commented May 27, 2017

Sorry, I found a few issues with the automatism for releasing and "lost my time". I will get back to this tomorrow at the earliest (releasing the previous, merging and releasing this).

This was referenced Jun 9, 2017

@ta2edchimp

This comment has been minimized.

Copy link
Collaborator

ta2edchimp commented Jun 13, 2017

Hey @randycoulman, sorry for the long break.
Could you please, for one last time, rebase your working branch on master and push your changes? When all tests still pass, we could then immediately merge and cut a release (v3.0.0).

feat(rule-finder): Omit deprecated rules
Filter out deprecated rules in `_getAllAvailableRules()` and
`_getPluginRules()`.  With this change, the lists of rules
displayed with the `-a` and `-u` flags will not include
deprecated rules.

Breaking: The output is now different from what it was, as
deprecated rules are now omitted by default.

Breaking: The `getRuleFinder()` API has changed.  Previously, it
took an optional filename and an optional boolean, `noCore`.
Now, it takes the optional filename and an options object:

* `omitCore`: Exclude core rules from the output
* `includeDeprecated`: Include deprecated rules in the output

Both options default to `false` if they are not provided.

Per a review request, I've added a flag to allow deprecated rules
to be included.  Adding `--include deprecated` or `-i deprecated`
will include deprecated rules in the output; they're not included
by default.

To make the implementation of this flag cleaner, I refactored
`getRuleFinder()` to take an options object instead of a boolean
as the second argument.  Adding a second boolean
(`includeDeprecated`) made it a difficult interface to use.

Note that `eslint.linter.getRules()` returns an ES6 Map, which
cannot be directly filtered.  Also, its `keys()` function returns
an iterable, so we have to spread that into an array before we
can filter it.  That's why the code is written the way it is.

To make this feature have a cleaner implementation, I also
performed the following refactorings:

* Don't pass plugin rules to `_getAllAvailableRules`.  Plugin
  rules are now merged with core rules directly in `RuleFinder`
  instead.

* Rename `_getAllAvailableRules` -> `_getCoreRules` to better
  reflect its purpose.

I also:

* Removed the `nc` option; it isn't documented and isn't being
  used.

* Split some of the `find` tests into two `it` blocks.  I found
  that calling `proxyquire` twice in the same test wasn't doing
  anything, so was potentially hiding failures.

* Renamed some `rule-finder` tests to result in more consistent
  output.

@randycoulman randycoulman force-pushed the randycoulman:omit-deprecated-rules branch from d777afd to 0dc7434 Jun 13, 2017

@randycoulman

This comment has been minimized.

Copy link
Contributor

randycoulman commented Jun 13, 2017

@ta2edchimp Done!

@randycoulman

This comment has been minimized.

Copy link
Contributor

randycoulman commented Jun 13, 2017

@ta2edchimp CI is failing because of test coverage. The line it's reporting as uncovered is function RuleFinder(specifiedFile, options = {}) {, which is definitely called a lot in the tests with various combinations of arguments, so I'm not sure why it's complaining.

@@ -103,6 +108,6 @@ function RuleFinder(specifiedFile, noCore) {
this.getUnusedRules = () => getSortedRules(unusedRules);
}

module.exports = function (specifiedFile, noCore) {
return new RuleFinder(specifiedFile, noCore);
module.exports = function (specifiedFile, options = {}) {

This comment has been minimized.

@ta2edchimp

ta2edchimp Jun 13, 2017

Collaborator

If you leave out the default value (... (specifiedFile, options) {) here, the tests will pass.
(It's left to RuleFinder then, to assign a default value).

This comment has been minimized.

@ta2edchimp

ta2edchimp Jun 13, 2017

Collaborator

CI is complaining, because the branch where RuleFinder assigns the default value to its options parameter never gets called (because the exported function already assigns an empty object as default value to option).

It is understandable, that this fails ... this should not have been passing previously.

This comment has been minimized.

@randycoulman

randycoulman Jun 13, 2017

Contributor

Yeah, that's it, thanks. I can't tell you how many times I've forgotten that there's that other function sitting there.

randycoulman added some commits Jun 13, 2017

feat(rule-finder): Remove default parameter value from RuleFinder
The default is supplied by the exported function, so doesn't need to be repeated here.
feat(rule-finder): Switch up where the default options are specified
Putting them on `RuleFinder` instead of the exported function so that they're closer to where they're used.

@ta2edchimp ta2edchimp merged commit 2e68098 into sarbbottam:master Jun 13, 2017

3 checks passed

codecov/patch 100% of diff hit (target 100%)
Details
codecov/project 100% (+0%) compared to 0cd85c8
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@@ -108,6 +108,6 @@ function RuleFinder(specifiedFile, options) {
this.getUnusedRules = () => getSortedRules(unusedRules);
}

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

This comment has been minimized.

@ljharb

ljharb Jun 13, 2017

Collaborator

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?

This comment has been minimized.

@randycoulman

randycoulman Jun 13, 2017

Contributor

@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.

This comment has been minimized.

@ta2edchimp

ta2edchimp Jun 13, 2017

Collaborator

Exactly that.

This comment has been minimized.

@ljharb

ljharb Jun 13, 2017

Collaborator

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

This comment has been minimized.

@ta2edchimp

ta2edchimp Jun 13, 2017

Collaborator

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.

This comment has been minimized.

@randycoulman

randycoulman Jun 13, 2017

Contributor

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

This comment has been minimized.

@ta2edchimp

ta2edchimp Jun 13, 2017

Collaborator

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.

This comment has been minimized.

@randycoulman

randycoulman Jun 13, 2017

Contributor

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.

This comment has been minimized.

@ta2edchimp

ta2edchimp Jun 13, 2017

Collaborator

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

This comment has been minimized.

@randycoulman

randycoulman Jun 13, 2017

Contributor

Done. #276

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Jun 13, 2017

@ta2edchimp please hold off before releasing; this should have been fixed prior to merging.

@randycoulman randycoulman deleted the randycoulman:omit-deprecated-rules branch Jun 13, 2017

@ta2edchimp

This comment has been minimized.

Copy link
Collaborator

ta2edchimp commented Jun 13, 2017

I think we covered everything. If I'm still missing something, please add a comment to #277

@randycoulman randycoulman referenced this pull request Feb 23, 2018

Merged

Prepare 3.2.1 #289

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment