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

Support allow option in no-unassigned-import, fix #671 #737

Merged
merged 4 commits into from
Jun 1, 2017

Conversation

kevin940726
Copy link
Contributor

Add allow option to no-unassigned-import to fix #671.

I basically follow the same logic from no-extraneous-dependencies. With the glob matching start from process.cwd(). I'm not sure that this is the best solution (maybe use regexp like webpack do?), but it works. Please let me know if there is something need to be improved. Big thanks!

  • write tests
  • implement feature/fix bug
  • update docs
  • make a note in change log

@coveralls
Copy link

coveralls commented Feb 5, 2017

Coverage Status

Coverage increased (+0.03%) to 94.941% when pulling 7a7763c on kevin940726:master into 790dd66 on benmosher:master.


This rule supports the following option:

`allow`: An Array of globs. The files that match any of these patterns would be ignored/allowed by the linter. This can be usefull for some build environment (e.g. css-loader in webpack).
Copy link
Member

Choose a reason for hiding this comment

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

s/usefull/useful, s/environment/environments

@@ -7,15 +9,40 @@ function report(context, node) {
})
}

function testIsAllow(globs, filename, source) {
if (!Array.isArray(globs)) {
return false // default doens't allow any pattern
Copy link
Member

Choose a reason for hiding this comment

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

s/doens't/doesn't, s/pattern/patterns

@@ -33,6 +60,7 @@ module.exports = {
'devDependencies': { 'type': ['boolean', 'array'] },
'optionalDependencies': { 'type': ['boolean', 'array'] },
'peerDependencies': { 'type': ['boolean', 'array'] },
'allow': { 'type': 'array' },
Copy link
Member

Choose a reason for hiding this comment

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

should this require that it be an array of strings specifically?

@kevin940726
Copy link
Contributor Author

Sorry for the typos, will change it next commit!

Yes, I think it should.

@coveralls
Copy link

coveralls commented Feb 5, 2017

Coverage Status

Coverage increased (+0.03%) to 94.941% when pulling b060805 on kevin940726:master into 790dd66 on benmosher:master.

@thiagodebastos
Copy link

Hey guys! Are we close to this being resolved? It looks like the only PR conflict is the CHANGELOG.md

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

LGTM pending a rebase

@ljharb
Copy link
Member

ljharb commented Apr 10, 2017

@kevin940726 please rebase on the command line so as to avoid merge commits; if you have trouble, i can do it for you.

@ljharb
Copy link
Member

ljharb commented Apr 10, 2017

(also; tests are failing)

@kevin940726
Copy link
Contributor Author

My bad, didn't notice that. I'll try my best. 😔

@kevin940726
Copy link
Contributor Author

Is the tests failing on master? Or what have I done wrong 😢

@ljharb
Copy link
Member

ljharb commented Jun 1, 2017

@kevin940726 sorry for the delay here. I've rebased your branch for you and resolved the conflicts. Tests seem to pass locally, so let's see if this will get them to pass.

@coveralls
Copy link

coveralls commented Jun 1, 2017

Coverage Status

Coverage increased (+0.03%) to 95.157% when pulling 8f9b403 on kevin940726:master into 28e1623 on benmosher:master.

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

Successfully merging this pull request may close these issues.

Add option to no-unassigned-import for allowed glob
4 participants