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.

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

fix(rule-finder): Support for unnamed scoped packages #302

Open
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@bradzacher
Copy link

bradzacher commented Jan 23, 2019

ESLint supports "unnamed" scoped plugins.
@scope translates to @scope/eslint-plugin.

This PR adds support for that case.

@ljharb
Copy link
Collaborator

ljharb left a comment

Looks great! could we add a test?

@sweetlikepete

This comment has been minimized.

Copy link

sweetlikepete commented Mar 29, 2019

Hey Brad, this fix is awesome!

I found something that might be a problem. When you run this version of eslint-find-rules over an eslint config that contains all the rules for @getify/eslint-plugin-proper-arrows all rules come up as missing.

I'm thinking this has something to do with this eslint-plugin using rule names in the format "@getify/proper-arrows/{rule-name}" instead of what every other plugin I've ever used does "proper-arrows/{ rule-name }".

Thanks again for updating this script. I hope this information is helpful.

@bradzacher

This comment has been minimized.

Copy link
Author

bradzacher commented Mar 29, 2019

Are you sure that's correct, @sweetlikepete?

Looking at the Readme for that plugin, it says you have to prefix the rule name with the scope

https://github.com/getify/eslint-plugin-proper-arrows/blob/master/README.md#eslintrcjson

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Mar 30, 2019

You definitely do; all scoped plugins and shareable configs work that way.

@sweetlikepete

This comment has been minimized.

Copy link

sweetlikepete commented Mar 30, 2019

It's the only eslint plugin I have that is part of a scoped package and when I run eslint-find-rules all the rules it has are returned as unused in the format 'proper-arrows/params proper-arrows/name' when they added in my eslint rules config as '@getify/proper-arrows/params' and '@getify/proper-arrows/name'. The configuration is working correctly in eslint.

],
"rules": {
"foo-rule": [2],
"old-rule": [2],
"plugin/foo-rule": [2],
"plugin/old-plugin-rule": [2],
"scoped-plugin/foo-rule": [2],
"scoped-plugin/old-plugin-rule": [2]
"scoped-plugin/old-plugin-rule": [2],

This comment has been minimized.

Copy link
@ljharb

ljharb Mar 31, 2019

Collaborator

this seems wrong to me; it should be @scoped-plugin/, not scoped-plugin/?

This comment has been minimized.

This comment has been minimized.

Copy link
@bradzacher

bradzacher Apr 5, 2019

Author

@DylanVann - typescript-eslint is a different case. it uses the special case name of @typescript-eslint/eslint-plugin which is automatically handled by eslint such that /eslint-plugin is not required anywhere.

However in this case, this looks like the old test cases were wrong.
Looking at an example of a package that doesn't use special naming (https://github.com/getify/eslint-plugin-proper-arrows#eslintrcjson), it should instead be @scope/scoped-plugin/old-plugin-rule

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 5, 2019

Collaborator

Right - the only thing that makes anything a scope is the leading @, ever

@bradzacher

This comment has been minimized.

Copy link
Author

bradzacher commented Apr 6, 2019

In regards to the failing tests - this is because @scope was only properly supported in ESLint v5.
Is it worth deleting the v4 test cases?
Or should we try and move them into a separate structure so v4 and v5 can be tested separately? (is it worth the effort considering v5.0.0 is almost a year old now?)

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Apr 6, 2019

Yes, it is still worth the effort to support as far back as we can.

Certainly if some functionality only is supported in eslint 5, we shouldn’t test it in < 5.

@bradzacher

This comment has been minimized.

Copy link
Author

bradzacher commented Apr 6, 2019

Ah small problem @ljharb.

By not testing scoped plugins/packages for < v5: the code coverage can never reach 100%, so the CI will always fail.
Off the top of my head, I don't think that there's any way to make this work without creating a file that's conditionally required?

Should I reconfigure travis to disable coverage checks for < v4?

bradzacher added some commits Apr 12, 2019

@bradzacher

This comment has been minimized.

Copy link
Author

bradzacher commented Apr 12, 2019

I used an istanbul comment to specifically disable coverage on the if statement in question, as it cannot be tested on <5 due to the differing implementations.

Note that I removed npm ls > /dev/null from the CI script, as it broke on an unrelated dependency on the node4 test.

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Apr 12, 2019

Why does npm ls break on node 4? It shouldn't.

@bradzacher

This comment has been minimized.

Copy link
Author

bradzacher commented Apr 12, 2019

https://travis-ci.org/sarbbottam/eslint-find-rules/jobs/519370506#L851

One of the dependencies (har-validator) of dependencies (request) of dependencies (all-contributors-cli) uses a different AJV version and it causes it to error.

I think that the dependency resolution algorithm must be different in node v4?

What's the reason that it was running npm ls?

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Apr 12, 2019

In that case, please add nvm install-latest-npm to before_install in travis.yml, rather than removing npm ls (which, if it fails, means that nothing can be expected to work, because the dependency graph is invalid)

@bradzacher

This comment has been minimized.

Copy link
Author

bradzacher commented Apr 12, 2019

Done! That worked, so it must have been a change in the super old npm algorithm.
All tests are passing now!

Show resolved Hide resolved .travis.yml
@ljharb

ljharb approved these changes Apr 12, 2019

@noamokman

This comment has been minimized.

Copy link

noamokman commented Apr 16, 2019

Any updates on this?

@ljharb ljharb requested review from sarbbottam and jfmengels Apr 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.