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

Implement extensions rule #250

Merged
merged 1 commit into from
Apr 22, 2016
Merged

Implement extensions rule #250

merged 1 commit into from
Apr 22, 2016

Conversation

lo1tuma
Copy link
Contributor

@lo1tuma lo1tuma commented Apr 20, 2016

This fixes #209 and fixes #204.


### Exception

When disallowing the use of certain extensions this rule makes an exception an allows the use of extension when the file would not be resolvable without extension.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exception an allows --> exception and allows

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, fixed.

@lo1tuma lo1tuma force-pushed the extensions branch 2 times, most recently from 3fa81d2 to b53639a Compare April 20, 2016 14:28
"object-assign": "^4.0.1",
"pkg-up": "^1.0.0"
"object-assign": "^4.0.1"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

object-assign is duplicated (and linked to that: there seems to be missing a comma after pkg-up).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed now.
Seems like I did a bad job at resolving the conflicts after rebasing.

@lo1tuma lo1tuma force-pushed the extensions branch 3 times, most recently from 7d09b76 to 2fc42a1 Compare April 20, 2016 14:36
@jfmengels
Copy link
Collaborator

jfmengels commented Apr 20, 2016

I think you can also add an entry to the changelog (and thank yourself).

Also, do you mind adding support for require? We had a discussion about a similar case here #245 (comment).

LGTM otherwise, nice job @lo1tuma!

@benmosher
Copy link
Member

@jfmengels is there a utility function in what you've added to filter CallExpression to just CommonJS require calls? I've got something off in my utils branch, but I thought you had something similar.

I'm imagining something so CommonJS support is as easy as:

return {
  ImportDeclaration: checkFileExtension,
  CallExpression: filterRequire(checkFileExtension),
}

Anyway @lo1tuma if you don't want to worry about CommonJS, I'm cool with that, we can do it post-merge.

@jfmengels
Copy link
Collaborator

Yeah, I have a function for that, with a use of it here. You can't do something as simple as @benmosher proposed, as the same data (the source for instance here) as not at the same property path. It's more test code than code.

we can do it post-merge

👍, I can probably take a look at it if you wish.

@benmosher
Copy link
Member

benmosher commented Apr 20, 2016

Ah, right. I made a moduleVisitor* to which you pass a function of the Literal node, here it is in use for no-unresolved: https://github.com/benmosher/eslint-plugin-import/blob/core-package/src/rules/no-unresolved.js

I really need to get that stuff merged back in. CJS vs. ES6 is cross-cutting and common enough that ideally, rules wouldn't need to worry about it explicitly.

*note: the JSDoc mistakenly documents the prototype of the visitor as Function(String), but it really does take the node object representing the string in the linted source.

@lo1tuma
Copy link
Contributor Author

lo1tuma commented Apr 21, 2016

I’m not very familiar with AMD so I could only provide an implementation for commonjs.

Apart from that, I think it would be better to wait for the moduleVisitor and not implementing the same logic in every rule.

@jfmengels
Copy link
Collaborator

jfmengels commented Apr 21, 2016

Sounds good to me

@benmosher benmosher mentioned this pull request Apr 21, 2016
@lo1tuma lo1tuma force-pushed the extensions branch 2 times, most recently from 9cf5d64 to dcf064a Compare April 22, 2016 17:06
@lo1tuma
Copy link
Contributor Author

lo1tuma commented Apr 22, 2016

I totally forgot about the CHANGELOG. Should be all good now.

@benmosher benmosher merged commit dcf064a into import-js:master Apr 22, 2016
benmosher added a commit that referenced this pull request Apr 22, 2016
@benmosher
Copy link
Member

Just merged, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants