Allow filename patterns for rule no-extraneous-dependencies. #527

knpwrs · 2016-08-27T19:14:58Z

Following the suggestion in #470 (comment).

Fixes #470.

coveralls · 2016-08-27T19:19:05Z

Coverage increased (+0.008%) to 97.737% when pulling d9b9730 on knpwrs:no-extraneous-dependencies-regex into 8da6a5a on benmosher:master.

coveralls · 2016-08-28T00:03:31Z

Coverage increased (+0.008%) to 97.737% when pulling c739798 on knpwrs:no-extraneous-dependencies-regex into 8da6a5a on benmosher:master.

jfmengels · 2016-08-28T21:11:00Z

docs/rules/no-extraneous-dependencies.md

+"import/no-extraneous-dependencies": ["error", {"devDependencies": /test|spec/}]
+```
+
+When using a regular expression the result of running [`test`] against the name of the file being linted is used as the boolean value. For example, the above configurations will allow the import of `devDependencies` in files whose names include `test` or `spec`.


When using a regular expression --> When using a regular expression, (+ comma)

the result of running [test] against the name of the file being linted is used as the boolean value. --> the setting will be activated if the name of the file being linted matches the given regular expression.

jfmengels · 2016-08-28T21:16:50Z

Hi @knpwrs! Thanks a lot for this, it looks great!

I'm asking myself now whether a glob configuration would not more appropriate (as we're adding minimatch in some other PR anyway), as I think it would give more control, and be easier to write than regex when things get a bit complicated. What do you think @benmosher @ljharb? If you're fine with only a regex (or waiting until requests come in and want to eventually support both regex and glob), then that's fine with me.

(sorry for being a party-pooper 😞)

coveralls · 2016-08-28T22:15:06Z

Coverage increased (+0.008%) to 97.737% when pulling 1be4720 on knpwrs:no-extraneous-dependencies-regex into 8da6a5a on benmosher:master.

knpwrs · 2016-08-28T22:19:42Z

Yeah, I saw the discussion about glob vs regular expression but didn't see if it went anywhere. I could see it going either way. I've used globs for everything filename related until webpack came along.

Might be overkill, but maybe it would be worth supporting both? Would it be safe to assume that a string bookended by / and / is a regular expression? Essentially:

'foo': glob (via string)
'/foo/': regex (via string)
/foo/: regex (literal, only from js configuration)
true or false: boolean (literal)

jfmengels · 2016-08-28T22:21:31Z

src/rules/no-extraneous-dependencies.js

-      'peerDependencies': { 'type': 'boolean' },
+      'devDependencies': { 'type': ['boolean', 'string', 'object'] },
+      'optionalDependencies': { 'type': ['boolean', 'string', 'object'] },
+      'peerDependencies': { 'type': ['boolean', 'string', 'object'] },


You're allowing object, because a regular expression value is an object, right? Just want confirmation.

Yeah, AFAIK the schema is checked with typeof, and typeof /foo/ === 'object'.

jfmengels · 2016-08-28T22:27:15Z

I like globs as you can decompose your allowance rules ['**/test/**.js', '*/*.test.js', '!**/fixtures/**']. Since you can do anything a regex does using this, I'd prefer this option. And having both options sounds like overkill, but why not, doesn't cost anything. But you're right that there may be a conflict in figuring out whether "test" is a glob or a regex...

And thanks for doing this, the bikeshedding may continue, but at least this is going forward :)

knpwrs · 2016-08-28T22:30:18Z

For two of those examples (**/test/**.js and !**/fixtures/**) I believe the current solution of a directory-level .eslintrc{,.yml,.js} is sufficient, no? The purpose this serves is for when test files live next to implementation files.

jfmengels · 2016-08-28T22:39:35Z

True, didn't think them through much ^^
But maybe you have a lot of directories in test/, each with their fixtures (or something else) subdirectory, in which the devdependency rule should be deactivated.

test/
  A/
    a.js
    a2.js
    fixtures/
      no-dev-deps.js
  B/
    b.js
    b2.js
    fixtures/
      no-dev-deps.js

If you have a lot of them, it gets tedious to add a .eslintrc file in each fixtures/ folder. You'd probably rather do that with ['**/test/**.js', '*/*.test.js', '!**/test/*/fixtures/**'].

Anyway, you'd make a clearer selection using globs than regexes IMO.

coveralls · 2016-08-28T23:29:55Z

Coverage increased (+0.02%) to 97.745% when pulling 8ff322d on knpwrs:no-extraneous-dependencies-regex into 8da6a5a on benmosher:master.

knpwrs · 2016-08-28T23:33:01Z

Kaboom. No kill like overkill! Looks like everything new is covered.

jfmengels · 2016-08-28T23:38:00Z

docs/rules/no-extraneous-dependencies.md

+"import/no-extraneous-dependencies": ["error", {"devDependencies": [/test/, '/spec/', '*.test.js', '*.spec.js']}]
+```
+
+When using an array of globs or regular expressions, the setting will be activated if the name of the file being linted matches a single glob or regular expression in the array.


matches a single glob or regular expression in the array

Hmm... Not sure this part works. Globs as an array allows you to specify multiple things ['test/**/*.js', 'src/**/*.js'], but also to forbid elements allowed by previous "expressions": ['test/**/*.js', 'src/**/*.js', '!fixtures']. Here <root>/test/fixtures/a.js should not work, whereas from your docs (and guessing implementation), it would.

(This is a glob right? Or am I confusing terms?)

Dang it. Back to the drawing board.

Maybe we should only support arrays for globs? But then what is excluded? Just things that contain an exclamation mark? In the case of ['*.test.js', '*.spec.js'], I'd want that to be additive. In the case of ['*.test.js', '!**/fixtures/**'], I'd want that to be subtractive.

This is approaching a point of complexity that may not be best for this plugin to handle. In your multiple test and fixture directory example we're already approaching a status of, "really, now?" At least IMHO.

A thought occurs. Maybe allow a function for configuration? Pass the name to the function and expect a boolean back. This plugin could support the most basic use cases and then people who need more complex functionality can implement it themselves.

Re: your closing question... Is what a glob? The string containing <root>? Not sure.

Also worth noting that passing a function obviously requires a JavaScript configuration file. I think that's a reasonable requirement if somebody requires super-specific rules.

['.test.js', '.spec.js'] I'd want that to be additive. In the case of ['.test.js', '!__/fixtures/*'], I'd want that to be subtractive.

That is the case. Imagine

[ '**/*.test.js', // All the file ending with '.test.js' 'test/**/*.js', // + All the files in test/ '!**/fixtures/**' // Take all the files selected by the previous two selectors, // and remove those in a fixtures (sub-)folder ]

This is approaching a point of complexity that may not be best for this plugin to handle.

I'm not sure I agree, but that's a subjective point a view ;)

Maybe allow a function for configuration

The function is doable, but then we're pushing the responsibility of doing the glob/regex logic to the user. I'd use functions as a last last resort.

These aren't ignore files - I don't think they need to be subtractive. There's nothing wrong with requiring people to nest .eslintrc files for things that can easily be achieved that way, but that would otherwise complicate the config.

benmosher · 2016-08-29T13:52:25Z

having both options sounds like overkill, but why not, doesn't cost anything

I think we've found the cost of both is in the design discussion. 😅

I think it should be one or the other. There is precedent for both at this point (pending the PR @jfmengels referenced).

My gut says it should be globs, going forward, whenever we're defining configuration for sets of files. My suspicion is that one or two globs will be sufficient in 99% of the cases where this is used, i.e. [ "**/*.spec.js" ].

So, I propose that it be an array of globs, handled by minimatch, until such time as it can be shown that there is a prevalent use case that is solved well and simply by regex and not globs.

knpwrs · 2016-08-29T20:10:26Z

So let's say we drop regex, how do we want to handle the additive / subtractive problem with globs?

jfmengels · 2016-08-29T20:39:50Z

@knpwrs Sorry, what problem?

knpwrs · 2016-08-29T20:55:30Z

I'm just currently thinking in the mindset of how I implemented the array functionality using Array.prototype.some. If a single pattern matches, then the whole test passes. Simple. With globs it has to match any given pattern in the array but then if there are any patterns with exclamation marks then it has to be checked against those as well and negated. We can't use Array.prototype.every because a given file shouldn't be expected to match every pattern. Right now my initial thought is to separate the patterns into positive and negative patterns and then test positive.some(/* filename test */) && !negative.some(/* filename test */). But maybe I'm overthinking it.

ljharb · 2016-08-29T20:56:49Z

That all seems overcomplicated. If you want to exempt a path, either use a nested .eslintrc, or write your "include" glob more precisely.

knpwrs · 2016-08-29T20:56:57Z

Unless there's something about glob composition I'm missing.

knpwrs · 2016-08-29T20:58:01Z

Right, okay, so that's going to a point I was making at #527 (comment). So is the current idea just to roll this back to nothing but matching some glob pattern?

ljharb · 2016-08-29T20:58:50Z

That's what makes the most sense to me.

knpwrs · 2016-08-29T21:00:08Z

Cool, I'll knock it out when I get home. 😄

coveralls · 2016-08-29T23:51:00Z

Coverage increased (+0.007%) to 97.736% when pulling 8d16c08 on knpwrs:no-extraneous-dependencies-regex into 8da6a5a on benmosher:master.

knpwrs · 2016-08-30T00:27:32Z

ɯooqɐʞ

ljharb · 2016-08-30T04:28:37Z

docs/rules/no-extraneous-dependencies.md

+You can also use globs instead of literal booleans:
+
+```js
+"import/no-extraneous-dependencies": ["error", {"devDependencies": "*.test.js"}]


it seems like it'd be simpler to just require this to always be an array - eslint can enforce it, and that way the user would get an immediate indication that multiple globs were possible.

Sure, I'll go ahead and do that.

coveralls · 2016-08-30T17:04:10Z

Coverage increased (+0.007%) to 97.736% when pulling 505aea8 on knpwrs:no-extraneous-dependencies-regex into 8da6a5a on benmosher:master.

coveralls · 2016-08-30T17:27:47Z

Coverage increased (+0.007%) to 97.736% when pulling 2b9b500 on knpwrs:no-extraneous-dependencies-regex into 8da6a5a on benmosher:master.

knpwrs · 2016-08-31T20:20:33Z

How's this looking now?

jfmengels · 2016-08-31T20:57:15Z

LGTM 😄

benmosher · 2016-09-01T10:41:18Z

tristanbbq · 2016-09-14T14:15:23Z

@benmosher
My settings:
"import/no-extraneous-dependencies": ["error", {"devDependencies": ['*.test.js', '*.spec.js']}]

Filename: SOMETHING.test.js - still get this 'should be listed in the projects dependencies, not devDependencies' - something I'm missing here??

benmosher · 2016-09-14T15:10:43Z

if it's deep, might need to be ['**/*.test.js'], otherwise it might only match if the file is in the CWD. not positive, though

benmosher · 2016-09-14T15:12:03Z

if that works, let me know--we should update the example in the docs to that

tristanbbq · 2016-09-14T15:14:38Z

@benmosher works 👍 - can confirm that glob '**/*.test.js' works but not '*.text.js'

Allow regular expressions for rule no-extraneous-dependencies.

d9b9730

Add RegExp documentation for no-extraneous-dependencies.

c739798

jfmengels reviewed Aug 28, 2016
View reviewed changes

Update doc due to review.

1be4720

jfmengels reviewed Aug 28, 2016
View reviewed changes

Extend no-extraneous-dependencies to support globs and arrays.

8ff322d

jfmengels reviewed Aug 28, 2016
View reviewed changes

Remove regular expression support from no-extraneous-dependencies.

8d16c08

knpwrs changed the title ~~Allow regular expressions for rule no-extraneous-dependencies.~~ Allow filename patterns for rule no-extraneous-dependencies. Aug 29, 2016

ljharb reviewed Aug 30, 2016
View reviewed changes

knpwrs added 2 commits August 30, 2016 12:25

Only support arrays of globs.

505aea8

Update comments for no-extraneous-dependencies configuration.

2b9b500

benmosher merged commit 2b9b500 into import-js:master Sep 1, 2016

benmosher added a commit that referenced this pull request Sep 1, 2016

Merge branch 'knpwrs/no-extraneous-dependencies-regex' (#527)

b6c1a33

benmosher mentioned this pull request Sep 14, 2016

[no-extraneous-dependencies] Add exceptions? #422

Closed

tristanbbq added a commit to tristanbbq/eslint-plugin-import that referenced this pull request Sep 14, 2016

import-js#527: Correct docs for rule no-extraneous-dependencies

271df10

tristanbbq mentioned this pull request Sep 14, 2016

#527: Correct docs for rule no-extraneous-dependencies #560

Merged

benmosher pushed a commit that referenced this pull request Sep 14, 2016

#527: Correct docs for rule no-extraneous-dependencies (#560)

c07c057

knpwrs mentioned this pull request Sep 15, 2016

The imports in tests for the rule import/no-extraneous-dependencies airbnb/javascript#959

Closed

This was referenced Apr 15, 2018

Update eslint-plugin-import to 2.12.0 sbardian/DebtTrackerAPI#25

Merged

Update dependency eslint-plugin-import to v2.11.0 Urigo/tortilla#68

Merged

Allow filename patterns for rule no-extraneous-dependencies. #527

Allow filename patterns for rule no-extraneous-dependencies. #527

Conversation

knpwrs commented Aug 27, 2016

coveralls commented Aug 27, 2016 • edited Loading

coveralls commented Aug 28, 2016 • edited Loading

Choose a reason for hiding this comment

jfmengels commented Aug 28, 2016 • edited Loading

coveralls commented Aug 28, 2016 • edited Loading

knpwrs commented Aug 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfmengels commented Aug 28, 2016 • edited Loading

knpwrs commented Aug 28, 2016

jfmengels commented Aug 28, 2016

coveralls commented Aug 28, 2016 • edited Loading

knpwrs commented Aug 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benmosher commented Aug 29, 2016

knpwrs commented Aug 29, 2016

jfmengels commented Aug 29, 2016

knpwrs commented Aug 29, 2016

ljharb commented Aug 29, 2016

knpwrs commented Aug 29, 2016

knpwrs commented Aug 29, 2016

ljharb commented Aug 29, 2016

knpwrs commented Aug 29, 2016 • edited Loading

coveralls commented Aug 29, 2016 • edited Loading

knpwrs commented Aug 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 30, 2016 • edited Loading

coveralls commented Aug 30, 2016 • edited Loading

knpwrs commented Aug 31, 2016

jfmengels commented Aug 31, 2016

benmosher commented Sep 1, 2016

tristanbbq commented Sep 14, 2016

benmosher commented Sep 14, 2016

benmosher commented Sep 14, 2016

tristanbbq commented Sep 14, 2016 • edited Loading

coveralls commented Aug 27, 2016 •

edited

Loading

coveralls commented Aug 28, 2016 •

edited

Loading

jfmengels commented Aug 28, 2016 •

edited

Loading

coveralls commented Aug 28, 2016 •

edited

Loading

knpwrs commented Aug 28, 2016 •

edited

Loading

jfmengels commented Aug 28, 2016 •

edited

Loading

coveralls commented Aug 28, 2016 •

edited

Loading

knpwrs commented Aug 29, 2016 •

edited

Loading

coveralls commented Aug 29, 2016 •

edited

Loading

coveralls commented Aug 30, 2016 •

edited

Loading

coveralls commented Aug 30, 2016 •

edited

Loading

tristanbbq commented Sep 14, 2016 •

edited

Loading