Implement new rule: no-internal-modules #485

spalger · 2016-08-12T01:40:38Z

coveralls · 2016-08-12T01:43:10Z

Coverage increased (+0.05%) to 97.721% when pulling 8a96faf on spalger:implement/rule/no-reaching-in into 90dedd7 on benmosher:master.

coveralls · 2016-08-12T01:48:37Z

Coverage increased (+0.05%) to 97.721% when pulling f072e8a on spalger:implement/rule/no-reaching-in into 90dedd7 on benmosher:master.

coveralls · 2016-08-12T01:50:59Z

Coverage increased (+0.05%) to 97.721% when pulling 4e9a2a1 on spalger:implement/rule/no-reaching-in into 90dedd7 on benmosher:master.

coveralls · 2016-08-12T02:00:34Z

Coverage increased (+0.06%) to 97.722% when pulling 7c3ea3d on spalger:implement/rule/no-reaching-in into 90dedd7 on benmosher:master.

coveralls · 2016-08-12T02:06:24Z

Coverage increased (+0.06%) to 97.722% when pulling 275ddc8 on spalger:implement/rule/no-reaching-in into 90dedd7 on benmosher:master.

coveralls · 2016-08-12T02:11:21Z

Coverage increased (+0.06%) to 97.726% when pulling a2dd4fd on spalger:implement/rule/no-reaching-in into 90dedd7 on benmosher:master.

benmosher · 2016-08-12T12:08:54Z

Ah, sounds cool. Normally I'd like to see an issue with a proposal first, but a PR makes for a very explicit proposal. 😄

Adding minimatch is a non-trivial dependency, as other settings/options may leverage some sort of glob evaluator, and they should all use the same one. It's worth discussing what (if any) glob parsers are used for other plugins / ESLint core (i.e. the .eslintignore file must use something).

Need to get @jfmengels' thoughts on this, too.

@sindresorhus, I think you may have thoughts an feelings on something like this, too, if you're willing to take the time to evaluate and share. (no worries if not!)

Also, I'm not sure I love no-reaching-inside as a name, but I don't immediately have a better one in mind. I'd probably go toward something positive, i.e. import/shallow or something. But that is a more half-baked thought. I think it is important the name be as clear as possible, though, as it will live as long as the rule does.

jfmengels · 2016-08-12T14:14:26Z

Thanks @spalger!

Normally I'd like to see an issue with a proposal first

Same here, but this sounds interesting anyway :)

Not sure about the rule name either. I like the idea of shallow, but starting with no- is more ESLint like.

I do think that ../foo/index should be allowed though.

Not very constructive, but gotta go sorry ^^'

spalger · 2016-08-12T19:29:05Z

Normally I'd like to see an issue with a proposal first

Sorry :) Happy to see it go in, but not married to the idea if y'all would prefer a different direction entirely.

I'd probably go toward something positive, i.e. import/shallow or something.

I like the idea, but one thing I don't want to imply is that the rule is missing a depth parameter, which is a feeling I get with the name "shallow". In my opinion the rule is sort of useless if you can reach into a module at all (with the exception being non-module directories).

Adding minimatch is a non-trivial dependency

I would love to replace minimatch with another solution. Especially if it allow removing the path-separator-normalization dance. The initial implementation followed the lead of no-restricted-paths but it's dependency on process.cwd() meant it didn't work with linter-eslint in atom.

benmosher · 2016-08-22T10:48:14Z

Could you refactor to use glob instead of minimatch? Looks like that's ESLint's globber of choice, I'd like to be consistent with them (and then, ideally, the dependency can be shared).

spalger · 2016-08-22T20:49:26Z

glob is about finding files on the file system that match the pattern, not about checking if a known path (the resolved path of an import statement) matches a pattern. minimatch is the matching library used by glob though, so the user experience should be exactly the same.

If you think it's necessary I can probably extract the minimatch pattern used by glob, but I think it would make more sense to depend on minimatch directly.

benmosher · 2016-08-23T08:26:56Z

Ah, got it. As you can tell, I'm not very familiar with either. Just reminded me of an old ESLint issue about gitignore vs eslintignore. That makes sense, now.

jfmengels · 2016-08-23T08:31:20Z

docs/rules/no-reaching-inside.md

+
+## Rule Details
+
+This rule has one option, `allow` which is an array of minimatch patterns to identify directories whose children can be imported explicitly.


Can you make minimatch patterns a link that links to documentation about minimatch patterns?

jfmengels · 2016-08-23T08:38:06Z

This looks pretty good to me. I still think that ../foo/index and ../foo/index.js should be allowed though, as that is what is happening most of the time under the hood when you require ../foo, and is then only a coding style preference.

spalger · 2016-08-23T20:16:45Z

@jfmengels In my eyes, the spirit of the rule is that the consumer of a module shouldn't be concerned with the implementation of the module. This includes where the entry point is.

If the module is using an index.js one day, moved to a single file the next, and ended up with a directory containing package.json and a custom main entry, the consumer shouldn't need to know.

…iles

spalger · 2016-08-23T22:32:06Z

@jfmengels I updated the rule so that allow now whitelists files that can be imported with reaching. It will first check for an allow pattern that matches the import statement, then an allow pattern that matches the resolved file path, and if either succeeds then the import will pass.

Because of this you could use allow: ["*/index{.js,}"] to support importing ../foo/index and ../foo/index.js but not ../foo/bar/index.js.

…tches

jfmengels · 2016-08-23T23:21:39Z

src/rules/no-reaching-inside.js

+    return !!find(allowRegexps, re => re.test(importPath))
+  }
+
+  // minimatch patterns are expected to use / path seperators, like import


nitpick: seperators --> separators 😅

I spell that word wrong everywhere, all the time, I think I need to setup autohotkey just for it.

jfmengels · 2016-08-23T23:28:20Z

Some nitpicks/code style improvements from me, but looks good to me otherwise :)

coveralls · 2016-08-24T00:05:01Z

Coverage increased (+0.1%) to 97.779% when pulling d0ce3a8 on spalger:implement/rule/no-reaching-in into 90dedd7 on benmosher:master.

jfmengels · 2016-09-06T14:16:20Z

I get the reasoning of not wanting to duplicate the plugin name, but I find import/no-internal-modules confusing as it makes me think that you're not allowed to have internal modules.

(PS : Bikeshedding is hard :D)

benmosher · 2016-09-06T14:21:23Z

@jfmengels I think that's a reasonable concern in general, but in this case it's not obvious to me what incorrect behavior that name would imply.

i.e. what would you expect not to be allowed, given no-internal-modules as a name, that is allowed? or vice versa?

This rule is pretty restrictive in that way. Seems like a fair characterization.

jfmengels · 2016-09-06T14:31:30Z

what would you expect not to be allowed, given no-internal-modules as a name, that is allowed

Well, I wouldn't expect much to be allowed I guess if this rule forbid internal modules altogether :p
But in my mind, that's what the rule name says, even if it doesn't make much sense, and it's a bit hard for me to understand that it forbids importing an internal module, even though I know what it does.

You're right that naming and functionality-wise it's very close to no-nodejs-modules, but that is less confusing because I can't create core nodejs modules (now that I say it, I hope that it's clear that core modules are meant).

Let's put it this way: When I read the rule name, I read no internal modules, not import no internal modules (I don't read the plugin name when I read the rule). If you think that most people read it in the second way, then the name is fine by me.

benmosher · 2016-09-06T14:48:18Z

I guess I want to err on the side of being concise, because absolutely no internal modules ever would be a wild world in which to try to code (and lint!).

jfmengels · 2016-09-06T15:04:32Z

Ok, let's go with that then.
@spalger Just want to make sure of something. what happens if I want to import lodash/fp or lodash/fp/get for instance? Do I need to put it in the allowed imports, or is that allowed by default?

spalger · 2016-09-06T22:06:42Z

You would need to add lodash/** or lodash/* to the allow patterns

…to implement/rule/no-reaching-in

coveralls · 2016-09-11T22:33:58Z

Coverage increased (+0.04%) to 97.837% when pulling 3d00542 on spalger:implement/rule/no-reaching-in into bd99bdf on benmosher:master.

benmosher · 2016-09-12T08:25:57Z

Thanks for merging from master! Could you go ahead and rename it to no-internal-modules?

spalger · 2016-09-12T17:04:17Z

Whoops, got distracted previously I guess 😄 . Updated!

coveralls · 2016-09-12T17:40:41Z

Coverage increased (+0.04%) to 97.837% when pulling 152dd37 on spalger:implement/rule/no-reaching-in into bd99bdf on benmosher:master.

coveralls · 2016-09-12T17:40:41Z

Coverage increased (+0.04%) to 97.837% when pulling 152dd37 on spalger:implement/rule/no-reaching-in into bd99bdf on benmosher:master.

benmosher · 2016-09-12T19:35:21Z

test-build.js

@@ -0,0 +1,2 @@
+require('babel-register')
+require('./tests/src/rules/no-internal-modules.js')


what is this file? looks like an impromptu describe.only?

jfmengels · 2016-09-12T19:48:48Z

Apart from @benmosher's comment, this looks good to me. Thanks a lot @spalger! 😄

coveralls · 2016-09-12T22:48:55Z

Coverage increased (+0.04%) to 97.837% when pulling b158a32 on spalger:implement/rule/no-reaching-in into bd99bdf on benmosher:master.

jfmengels

LGTM
(sorry, just wanted to use the new GitHub approval system :p)

benmosher · 2016-09-15T10:11:53Z

lol @jfmengels, me too. @spalger: looks great! thanks for working with us!

spalger · 2016-09-15T19:01:00Z

🎉 🎉 🎉 🎉 🎉

jfmengels · 2016-09-15T19:06:25Z

Hey, once again: Thanks a lot @spalger, this is awesome!

Implement new rule: no-reaching-inside

f072e8a

normalize path segments all path segments to '/'

a2dd4fd

benmosher added rule proposal style-guide semver-minor labels Aug 22, 2016

jfmengels reviewed Aug 23, 2016
View reviewed changes

spalger added 2 commits August 23, 2016 13:20

point to minimatch/glob docs to define glob syntax

4530ae6

refactor no-reaching-inside, allow now points to importable modules/f…

f872281

…iles

add test to verify that "no-reaching/allow" can limit the depth of ma…

d0ce3a8

…tches

jfmengels reviewed Aug 23, 2016
View reviewed changes

Merge branch 'master' of github.com:benmosher/eslint-plugin-import in…

3d00542

…to implement/rule/no-reaching-in

rename to no-internal-modules

152dd37

spalger changed the title ~~Implement new rule: no-reaching-inside~~ Implement new rule: no-internal-modules Sep 12, 2016

benmosher reviewed Sep 12, 2016
View reviewed changes

benmosher removed the name-bikeshed-tbd label Sep 12, 2016

jfmengels mentioned this pull request Sep 12, 2016

release v1.15.0 #556

Merged

4 tasks

remove extra test executor

b158a32

benmosher added this to the 1.16.0 milestone Sep 13, 2016

jfmengels approved these changes Sep 14, 2016

View reviewed changes

benmosher approved these changes Sep 15, 2016

View reviewed changes

benmosher merged commit c57beae into import-js:master Sep 15, 2016

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

changelog note for #485

2b471c7

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


		## Rule Details

		This rule has one option, `allow` which is an array of minimatch patterns to identify directories whose children can be imported explicitly.

		@@ -0,0 +1,2 @@
		require('babel-register')
		require('./tests/src/rules/no-internal-modules.js')

Implement new rule: no-internal-modules #485

Implement new rule: no-internal-modules #485

Conversation

spalger commented Aug 12, 2016 • edited Loading

coveralls commented Aug 12, 2016 • edited Loading

coveralls commented Aug 12, 2016 • edited Loading

coveralls commented Aug 12, 2016 • edited Loading

coveralls commented Aug 12, 2016 • edited Loading

coveralls commented Aug 12, 2016 • edited Loading

coveralls commented Aug 12, 2016 • edited Loading

benmosher commented Aug 12, 2016

jfmengels commented Aug 12, 2016

spalger commented Aug 12, 2016 • edited Loading

benmosher commented Aug 22, 2016

spalger commented Aug 22, 2016 • edited Loading

benmosher commented Aug 23, 2016

jfmengels Aug 23, 2016

Choose a reason for hiding this comment

jfmengels commented Aug 23, 2016

spalger commented Aug 23, 2016 • edited Loading

spalger commented Aug 23, 2016 • edited Loading

jfmengels Aug 23, 2016

Choose a reason for hiding this comment

spalger Aug 24, 2016

Choose a reason for hiding this comment

jfmengels commented Aug 23, 2016

coveralls commented Aug 24, 2016

jfmengels commented Sep 6, 2016

benmosher commented Sep 6, 2016

jfmengels commented Sep 6, 2016

benmosher commented Sep 6, 2016

jfmengels commented Sep 6, 2016

spalger commented Sep 6, 2016

coveralls commented Sep 11, 2016 • edited Loading

benmosher commented Sep 12, 2016

spalger commented Sep 12, 2016

coveralls commented Sep 12, 2016

coveralls commented Sep 12, 2016 • edited Loading

benmosher Sep 12, 2016 • edited Loading

Choose a reason for hiding this comment

spalger Sep 12, 2016

Choose a reason for hiding this comment

jfmengels commented Sep 12, 2016

coveralls commented Sep 12, 2016 • edited Loading

jfmengels left a comment

Choose a reason for hiding this comment

benmosher commented Sep 15, 2016

spalger commented Sep 15, 2016

jfmengels commented Sep 15, 2016

spalger commented Aug 12, 2016 •

edited

Loading

coveralls commented Aug 12, 2016 •

edited

Loading

coveralls commented Aug 12, 2016 •

edited

Loading

coveralls commented Aug 12, 2016 •

edited

Loading

coveralls commented Aug 12, 2016 •

edited

Loading

coveralls commented Aug 12, 2016 •

edited

Loading

coveralls commented Aug 12, 2016 •

edited

Loading

spalger commented Aug 12, 2016 •

edited

Loading

spalger commented Aug 22, 2016 •

edited

Loading

spalger commented Aug 23, 2016 •

edited

Loading

spalger commented Aug 23, 2016 •

edited

Loading

coveralls commented Sep 11, 2016 •

edited

Loading

coveralls commented Sep 12, 2016 •

edited

Loading

benmosher Sep 12, 2016 •

edited

Loading

coveralls commented Sep 12, 2016 •

edited

Loading