make rule no-useless-path-segments work with commonjs
#1128
Conversation
|
cc @graingert |
|
Nice. It was totally my intention when writing this that it would work for commonjs too. Thanks for the cc @ljharb |
|
@ljharb we could make it a config option if we're being really strict with breaking changes |
|
Yeah that’s a good point - worst case we can put it behind an option. |
|
Please review again, now it's an option, so it can safely be a patch. |
| @@ -39,6 +39,8 @@ module.exports = { | |||
| url: docsUrl('no-useless-path-segments'), | |||
| }, | |||
|
|
|||
| schema: [ makeOptionsSchema() ], | |||
There was a problem hiding this comment.
this isn’t defining “commonjs” as an option tho
There was a problem hiding this comment.
doesn't line 111 from this function define it?
https://github.com/benmosher/eslint-plugin-import/blob/37554fe9844986a1ab126d2a8f59fe4c7551a2f8/utils/moduleVisitor.js#L107-L122
There was a problem hiding this comment.
oh, hmm. I don’t think it’s a good idea to add a bunch of options that aren’t being used. Can we make a schema that only has the commonjs option?
There was a problem hiding this comment.
from what i observe, every option is valid, function makeOptionsSchema is made specifically for rule that uses util moduleVisitor
checkout
https://github.com/benmosher/eslint-plugin-import/blob/37554fe9844986a1ab126d2a8f59fe4c7551a2f8/utils/moduleVisitor.js#L13-L101
but it's your call, i can add only commonjs
There was a problem hiding this comment.
it’s fine to add support for every option, but then let’s make sure to add tests for all the options.
|
@ljharb now there is one simple option :) |
|
We probably want to check amd too
…On Sun, 1 Jul 2018, 17:46 Jordan Harband, ***@***.***> wrote:
***@***.**** approved this pull request.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1128 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAZQTJJxGPszRaFq3FrPP1iVZnLZUuPDks5uCPzegaJpZM4U9GDp>
.
|
|
Sure, we just need a test for it. |
|
@1pete ^ |
|
I'm going to merge this now; if we want to add AMD let's do that in a followup PR. |
fixes #1127