Add rule 'no-self-import' #727

giodamelio · 2017-01-22T23:01:05Z

Stops a file from importing itself. See #449 for details.

Progress:

A few questions

Is there any special way the error message should be written?
Should I include the name of the file in it?
Is it best practices to dynamically generate error messages based on the context?

Closes #449. Fixes #447.

coveralls · 2017-01-22T23:07:01Z

Coverage increased (+0.04%) to 94.949% when pulling 411af62 on giodamelio:feature-no-self-import into b939718 on benmosher:master.

ljharb · 2017-01-22T23:05:55Z

src/rules/no-self-import.js

+  if (filePath === resolvedPath) {
+    context.report({
+        node,
+        // TODO: check in about message, should it print the name of the module itself


yes, i think it should - it's much more helpful that way. Ideally, it'd include the path to the file, and, if different, the path used in the import statement.

ljharb · 2017-01-22T23:07:22Z

tests/src/rules/no-self-import.js

+
+ruleTester.run('no-self-import', rule, {
+  valid: [
+    test({ code: 'import _ from "lodash"'}),


how can any of these valid examples be deemed such without a filename to test against?

It's probably good to keep all of these, to ensure that when there's no filename (like stdin input), the rule bails out, but these should include a filename explicitly.

Thanks, that makes sense, I don't totally grok the testing setup yet. I'll add that.

ljharb · 2017-01-22T23:07:36Z

src/rules/no-self-import.js

+import isStaticRequire from '../core/staticRequire'
+
+function isImportingSelf(context, node, requireName) {
+  const filePath = context.getFilename()


what happens when the input is from stdin, and thus there's no filename?

Good catch! When that happens, filePath will equal '<text>. In some other plugin, we've handled it like: https://github.com/sindresorhus/eslint-plugin-unicorn/blob/master/rules/filename-case.js#L71-L73

That seems totally reasonable - it might also be useful to create a generic way to tag rules as "does not apply when the file is from stdin"

I have actually never used the --stdin option and don't know the uses for it 🤔

I didn't even know that was a thing you could do.

I use it all the time when providing reproduction steps for eslint bugs :-p

last I knew, Sublime uses --stdin, but with --stdin-filename (or something like that) to fake in the filename

jfmengels

Thanks a lot @giodamelio, that was quick! 😄

Is there any special way the error message should be written?

We don't have a convention for error message in this plugin AFAIK. From what I see at a glance, most error messages end with a period (with some exceptions, but you might as well add it, I usually add it in other rules plugins) and start with an uppercase letter. As far as the content of the message, the clearer the better, but it should stay relatively short. The current message looks good to me, but if someone finds something better, 👍

Is it best practices to dynamically generate error messages based on the context?

If it helps the user understand and fix the error, sure.

Tests-wise, could you add the following cases:
valid:

filename is bar.js, code is importing ./bar/index.js
filename is bar/index.js, code is importing ./bar
invalid:
filename is bar/index.js, code is importing ../bar/
filename is index.js, code is importing ./
filename is index.js, code is importing .
filename is index.js, code is importing ././././
filename is bar.js, code is importing ./bar.js

And more if you think of more (odd) cases.

jfmengels · 2017-01-22T23:07:01Z

src/rules/no-self-import.js

+    context.report({
+        node,
+        // TODO: check in about message, should it print the name of the module itself
+        message: 'Module imports itself',


should it print the name of the module itself

I think there is no need. If they see the error in the editor, they will already have the file open and see which file the error is for. If they see it in the terminal or a CI environment's terminal, ESLint will display the location of the file it is in.

That's true - but because resolution is being applied, it might not be immediately apparent which import path is pointing back to the file.

ESLint will tell you on what line the import is, and in the editor, it will put red squiggly lines under the import statement / require call. That sounds sufficient to me, especially since (IMO) having the whole path to the file in the error message would probably make the error harder to read.

jfmengels · 2017-01-22T23:07:15Z

src/rules/no-self-import.js

+      description: 'Prohibits a file from importing itself',
+      recommended: true,
+    },
+    fixable: 'code',


Remove this line as the rule is not fixable.

giodamelio · 2017-01-23T00:04:04Z

That should take care of the changes you guys requested.

coveralls · 2017-01-23T00:05:22Z

Coverage increased (+0.04%) to 94.955% when pulling 5e133c0 on giodamelio:feature-no-self-import into b939718 on benmosher:master.

coveralls · 2017-01-23T00:05:22Z

Coverage increased (+0.04%) to 94.955% when pulling 5e133c0 on giodamelio:feature-no-self-import into b939718 on benmosher:master.

coveralls · 2017-01-23T00:05:22Z

Coverage increased (+0.04%) to 94.955% when pulling 5e133c0 on giodamelio:feature-no-self-import into b939718 on benmosher:master.

coveralls · 2017-01-23T00:09:04Z

Coverage increased (+0.04%) to 94.955% when pulling 5948d35 on giodamelio:feature-no-self-import into b939718 on benmosher:master.

coveralls · 2017-01-23T06:45:25Z

Coverage increased (+0.04%) to 94.955% when pulling 75d468c on giodamelio:feature-no-self-import into b939718 on benmosher:master.

coveralls · 2017-01-23T06:45:25Z

Coverage increased (+0.04%) to 94.955% when pulling 75d468c on giodamelio:feature-no-self-import into b939718 on benmosher:master.

giodamelio · 2017-01-23T06:46:54Z

That should cover the changelog and docs.

coveralls · 2017-01-23T06:50:24Z

Coverage increased (+0.04%) to 94.955% when pulling 57dd625 on giodamelio:feature-no-self-import into b939718 on benmosher:master.

giodamelio · 2017-01-23T07:01:21Z

I think the AppVeyor build might have failed because I pushed a second commit while the first one was still running, I saw that happen earlier. I didn't change any code in these last two commits, so it shouldn't have broken.

jfmengels

Do you need to have these files created fr the tests to be run correctly?

giodamelio · 2017-01-23T18:44:43Z

Yep those files are needed. I just added a comment in them explaining.

jfmengels

Yep those files are needed. I just added a comment in them explaining.

Perfect!

I added a few commits to fix some typos (don't force push over it!)

This LGTM! I'm leaving some time for someone else to review.

Oh, and no need for squashing it yourself, we can do it directly now with GitHub.

Again, thanks a lot @giodamelio!

giodamelio · 2017-01-23T19:06:51Z

Sounds good to me.

ljharb · 2017-01-23T19:11:19Z

(heads up tho, if you squash directly on github you actually lose the link to the git log for the PR - it's far better to rebase/squash on the command line before clicking the button on github)

coveralls · 2017-01-23T20:29:39Z

Coverage increased (+0.06%) to 94.969% when pulling 6c7775c on giodamelio:feature-no-self-import into e137b98 on benmosher:master.

coveralls · 2017-01-23T21:03:22Z

Coverage decreased (-1.3%) to 93.655% when pulling dd3f0d0 on giodamelio:feature-no-self-import into e137b98 on benmosher:master.

coveralls · 2017-01-23T21:03:22Z

Coverage decreased (-1.3%) to 93.655% when pulling dd3f0d0 on giodamelio:feature-no-self-import into e137b98 on benmosher:master.

coveralls · 2017-01-23T22:10:53Z

Coverage increased (+0.03%) to 94.947% when pulling d25cdd9 on giodamelio:feature-no-self-import into e137b98 on benmosher:master.

giodamelio · 2017-01-23T22:16:06Z

Oh my god, @coveralls bot is getting a bit annoying.

coveralls · 2017-01-23T22:23:44Z

Coverage decreased (-1.3%) to 93.655% when pulling d25cdd9 on giodamelio:feature-no-self-import into e137b98 on benmosher:master.

coveralls · 2017-01-23T22:23:45Z

Coverage increased (+0.03%) to 94.947% when pulling d25cdd9 on giodamelio:feature-no-self-import into e137b98 on benmosher:master.

coveralls · 2017-01-23T23:04:54Z

Coverage increased (+0.03%) to 94.947% when pulling d25cdd9 on giodamelio:feature-no-self-import into e137b98 on benmosher:master.

jfmengels · 2017-02-16T22:00:51Z

(heads up tho, if you squash directly on github you actually lose the link to the git log for the PR - it's far better to rebase/squash on the command line before clicking the button on github)

@ljharb I'm sorry, what git log are you talking about?
I was about to merge this, but I don't know how to squash this locally and then re-push to GitHub, as I don't have control over @giodamelio's fork. What do you recommend otherwise, squashing with the button anyway, or squashing on the command line and pushing to master?

Sorry about the delay @giodamelio!

giodamelio · 2017-02-16T22:03:30Z

I think he meant when you squash on the cli you can include the commit messages from all the commits in the body of the squashed commit. I can squash it for you if you want, but I don't really mind losing the messages.

jfmengels · 2017-02-16T22:15:42Z

Hmm, this is what I'd get if I were to squash it from GitHub (I have the option to edit if I want to):

* Add rule itself

* Add test

* Add requested changes

* Change the word prohibit to forbid to match other rules

* Update docs

* Update changelog

* Add comment in test files

* Fix typos

* Simplify implementation a bit

* Add thanks :)

* extra comma

So I'm not sure what logs would be lost.
But yeah sure, if you feel like squashing @giodamelio, might as well :)

giodamelio · 2017-02-16T22:18:13Z

Oh, I didn't think Github could do that. You might as well do it on your end then, the squash was giving me a bit of trouble and I would rather Github take care of it then me borking something.

ljharb · 2017-02-17T00:38:56Z

@jfmengels i'm talking about when you fetch the origin/pr/727 ref from the command line - that "ref" (a git concept) will be orphaned when using squashmerge or rebasemerge on the github UI.

In other words, it's not just the commit messages. I'm content to do the squash for you, if that's necessary?

coveralls · 2017-02-17T00:39:29Z

Coverage increased (+0.03%) to 94.949% when pulling e695506 on giodamelio:feature-no-self-import into b5a962f on benmosher:master.

ljharb · 2017-02-17T00:40:42Z

@jfmengels also, if you note the text "Add more commits by pushing to the feature-no-self-import branch on giodamelio/eslint-plugin-import." - PRs now by default have the box checked that allows maintainers to push branches directly to the fork - so in fact you can directly update this PR's branch, which then reruns the tests, before actually doing the merge.

ljharb · 2017-02-17T00:44:04Z

I've just done the rebase and force pushed it to the fork. Once tests have passed, please feel free to do a normal merge with a merge commit.

giodamelio · 2017-02-17T01:54:41Z

Not sure exactly why the AppVeyor build failed, I don't think it handled the rebase well.

coveralls · 2017-02-17T02:17:31Z

Coverage increased (+0.03%) to 94.949% when pulling 0ea3d80 on giodamelio:feature-no-self-import into b5a962f on benmosher:master.

coveralls · 2017-02-17T02:17:31Z

Coverage increased (+0.03%) to 94.949% when pulling 0ea3d80 on giodamelio:feature-no-self-import into b5a962f on benmosher:master.

giodamelio · 2017-08-01T07:23:16Z

@benmosher @jfmengels I just wanted to ping you guys and see about getting this merged and inquire about a new release, that could hopefully include this.

ljharb · 2017-08-01T07:25:12Z

@giodamelio it'll need another rebase to remove the merge commit (the "update branch" button doesn't help)

coveralls · 2017-08-01T07:40:29Z

Coverage increased (+0.02%) to 95.99% when pulling 6dc8a86 on giodamelio:feature-no-self-import into dd28130 on benmosher:master.

giodamelio · 2017-08-01T07:43:24Z

Yep, I shouldn't have used the "fix conflicts" button. Only causes me more issues 😑. Fix incoming.

giodamelio · 2017-08-01T07:49:58Z

Should be good to go.

coveralls · 2017-08-01T08:04:31Z

Coverage increased (+0.02%) to 95.99% when pulling fdb7823 on giodamelio:feature-no-self-import into dd28130 on benmosher:master.

giodamelio · 2017-10-13T02:24:36Z

Hey guys, I totally forgot about this pull. I think it should be ready to go still.

ljharb · 2017-10-13T06:19:38Z

@giodamelio would you rebase it on the command line, and we do need to figure out why the tests are failing

ljharb · 2017-10-13T06:19:52Z

(it's possible that tests are failing on master, tho)

coveralls · 2018-01-04T06:16:32Z

Coverage increased (+0.02%) to 96.2% when pulling be5b52d on giodamelio:feature-no-self-import into 523789f on benmosher:master.

ljharb · 2018-01-04T06:34:26Z

Took care of the rebase; tests are passing now.

giodamelio · 2018-01-04T06:36:27Z

Thanks, I totally forgot about this again :). Merged after almost a year. :)

[New] Add rule no-self-import

be5b52d

giodamelio mentioned this pull request Jan 22, 2017

Added no-self-import rule #449

Closed

ljharb reviewed Jan 22, 2017

View reviewed changes

jfmengels requested changes Jan 22, 2017

View reviewed changes

jfmengels reviewed Jan 23, 2017

View reviewed changes

jfmengels approved these changes Jan 23, 2017

View reviewed changes

ljharb force-pushed the feature-no-self-import branch from e695506 to 0ea3d80 Compare February 17, 2017 00:43

giodamelio force-pushed the feature-no-self-import branch from 6dc8a86 to fdb7823 Compare August 1, 2017 07:48

ljharb approved these changes Aug 1, 2017

View reviewed changes

ljharb added the rule proposal label Jan 4, 2018

ljharb force-pushed the feature-no-self-import branch from df1d5cd to be5b52d Compare January 4, 2018 05:51

ljharb merged commit a9bee1a into import-js:master Jan 4, 2018

ljharb mentioned this pull request Jan 4, 2018

"RangeError: Maximum call stack size exceeded" when file includes itself #210

Closed

renovate bot mentioned this pull request Feb 22, 2018

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

Merged

renovate bot mentioned this pull request Mar 22, 2018

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

Merged

Add rule 'no-self-import' #727

Add rule 'no-self-import' #727

Conversation

giodamelio commented Jan 22, 2017 • edited by ljharb Loading

coveralls commented Jan 22, 2017 • edited Loading

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

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

jfmengels left a 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

Choose a reason for hiding this comment

giodamelio commented Jan 23, 2017

coveralls commented Jan 23, 2017

coveralls commented Jan 23, 2017

coveralls commented Jan 23, 2017 • edited Loading

coveralls commented Jan 23, 2017 • edited Loading

coveralls commented Jan 23, 2017

coveralls commented Jan 23, 2017 • edited Loading

giodamelio commented Jan 23, 2017

coveralls commented Jan 23, 2017 • edited Loading

giodamelio commented Jan 23, 2017

jfmengels left a comment

Choose a reason for hiding this comment

giodamelio commented Jan 23, 2017

jfmengels left a comment

Choose a reason for hiding this comment

giodamelio commented Jan 23, 2017

ljharb commented Jan 23, 2017

coveralls commented Jan 23, 2017 • edited Loading

coveralls commented Jan 23, 2017

coveralls commented Jan 23, 2017

coveralls commented Jan 23, 2017 • edited Loading

giodamelio commented Jan 23, 2017

coveralls commented Jan 23, 2017

coveralls commented Jan 23, 2017 • edited Loading

coveralls commented Jan 23, 2017 • edited Loading

jfmengels commented Feb 16, 2017

giodamelio commented Feb 16, 2017

jfmengels commented Feb 16, 2017

giodamelio commented Feb 16, 2017

ljharb commented Feb 17, 2017

coveralls commented Feb 17, 2017 • edited Loading

ljharb commented Feb 17, 2017

ljharb commented Feb 17, 2017

giodamelio commented Feb 17, 2017

coveralls commented Feb 17, 2017

coveralls commented Feb 17, 2017 • edited Loading

giodamelio commented Aug 1, 2017

ljharb commented Aug 1, 2017

coveralls commented Aug 1, 2017 • edited Loading

giodamelio commented Aug 1, 2017

giodamelio commented Aug 1, 2017

coveralls commented Aug 1, 2017 • edited Loading

giodamelio commented Oct 13, 2017

ljharb commented Oct 13, 2017

ljharb commented Oct 13, 2017

coveralls commented Jan 4, 2018 • edited Loading

ljharb commented Jan 4, 2018

giodamelio commented Jan 4, 2018

giodamelio commented Jan 22, 2017 •

edited by ljharb

Loading

coveralls commented Jan 22, 2017 •

edited

Loading

coveralls commented Jan 23, 2017 •

edited

Loading

coveralls commented Jan 23, 2017 •

edited

Loading

coveralls commented Jan 23, 2017 •

edited

Loading

coveralls commented Jan 23, 2017 •

edited

Loading

coveralls commented Jan 23, 2017 •

edited

Loading

coveralls commented Jan 23, 2017 •

edited

Loading

coveralls commented Jan 23, 2017 •

edited

Loading

coveralls commented Jan 23, 2017 •

edited

Loading

coveralls commented Feb 17, 2017 •

edited

Loading

coveralls commented Feb 17, 2017 •

edited

Loading

coveralls commented Aug 1, 2017 •

edited

Loading

coveralls commented Aug 1, 2017 •

edited

Loading

coveralls commented Jan 4, 2018 •

edited

Loading