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

could import/no-extraneous-dependencies get a path regex for dev deps? #470

Closed
ljharb opened this issue Aug 2, 2016 · 13 comments
Closed

Comments

@ljharb
Copy link
Member

ljharb commented Aug 2, 2016

Currently, the airbnb config only allows deps, and that means that everyone has to manually do something like https://github.com/airbnb/javascript/blob/master/packages/eslint-config-airbnb-base/test/.eslintrc#L8-L10 for their tests.

It would be ideal if in the shared config, we could specify that folders named "test" or "spec", for example, could have dev deps, but nothing else. I think that would cover the majority use case and remove the need for most devs to have to override this rule.

@benmosher
Copy link
Member

Yeah, something like that would be really helpful. I started with all my tests in test/ next to src/ but have been playing with putting them closer to code, and I know Jest defaults (used to default?) to __tests__ right next to code files. .eslintrc would be exhausting in such a case.

I noticed lately that eslint-plugin-node addresses a similar problem by allowing devDependencies from files that are not .npmignore'd or are covered by files in the package.json, which could be another cool way to infer it, at least for projects with a package.json.

@jfmengels do I remember you mentioning that ESLint proper will allow file-path-regex-based config soon? It would be ideal to have first-party support for this.

@jfmengels
Copy link
Collaborator

That has been discussed in eslint/eslint#3611 and eslint/eslint#5040. I get the feeling that that is definitely the direction that ESLint wants to go, but I would not go as far as soon.

It's definitely something to look forward to, as it would remove a lot of configuration problems.

It would be ideal to have first-party support for this.

Not sure what you mean with this, as this way of configuring would leave all the work to the user. Rules would be disabled or enabled differently where needed, and plugins won't have to do anything to support that.

@benmosher
Copy link
Member

Right, yeah, in this case I meant ESLint proper is the first party, and we're the third party.

airbnb config (as the second party?) could then spec regex in the shared config that would be fed to ESLint, instead of us.

But yeah, if that's not soon... what if the flag values could be true/false/regex (string for YAML/json) and if regex, pattern.test(context.getFilename()) becomes the setting?

i.e.

//.eslintrc.js or shared config
exports.rules = {
  'import/no-extraneous-dependencies': [2, { devDependencies: /test|spec/ }],
}

I'm think I'm just restating what you've suggested, in the interest of checking consensus. This would be simpler to implement than what I previously suggested a la eslint-plugin-node.

@ljharb
Copy link
Member Author

ljharb commented Aug 2, 2016

That sounds great, and would help people using jest as well.

Ideally the getFilename() tested would use the full path name from where eslint was invoked - ie, not foo.js but test/a/b/c/foo.js so that I could use /^test\/|^spec\/|__test__\// as the regex, for example.

@benmosher
Copy link
Member

benmosher commented Aug 2, 2016

getFilename() tested would use the full path name from where eslint was invoked

context.getFilename() is actually the full absolute path to the file currently being linted, which I think is the right path to test (since it's the one to be allowed to import or not from devDeps).

Could theoretically truncate to just the part after the CWD but in general, absolute paths tend to help avoid weird editor-linter integration issues and it's not obvious to me that /^test\// is substantially more useful than /\/test\//.

Or is it? Am I off here? It would be more precise, certainly, but again, lots of issues with pathing when running inside of Sublime/Atom.

@ljharb
Copy link
Member Author

ljharb commented Aug 2, 2016

Personally, I'd only want the top-level "test" directory exempted - in my repos, tests are never colocated with implementation, and thus I wouldn't want someone to make a "test" directory inside production code and suddenly have test-related linter rules applied.

I'm not sure what issues you're thinking of - Sublime, at least, can use the project root, and process.cwd() inside eslint should be consistent with the PWD where eslint was ran?

@benmosher
Copy link
Member

I agree, I'd only want the top one as well, and it would be ideal to be that precise.

And with some massaging, the Sublime issues can be resolved.

I'm just playing it out on account of old scars from dealing with nonsense around the CWD. The absolute path to the file is well-known regardless of how/where/why/when the rule is being run.

Sans-CWD would be the only way for ^ to be consistently meaningful, though. For that reason, it's probably useful to do it that way, and the CWD issues can be resolved.

@benmosher
Copy link
Member

Pivot: would a glob be better than regex?

@jfmengels
Copy link
Collaborator

Frankly, I don't think this is something that we should take care of, as this is really related to configuring ESLint. The AVA and the Mocha plugins have managed to avoid this kind of setting until now.

I think that putting an extra .eslintrc file at the root of the test directory is the way to go. In the case of the airbnb config, which can't enforce/apply that, it's harder, but that would still be my suggestion.

I'd at least contact the ESLint maintainers on the issue or on Gitter and ask them how far along it is before trying to implement this.

@ljharb
Copy link
Member Author

ljharb commented Aug 2, 2016

@jfmengels that's what I do now, but that's not something I can specify in a shared config :-/ and because rule overrides aren't delta-based, but rather complete replacements, someone would have to dive into node_modules to figure out what the current settings are for a rule, copy-paste them, and THEN modify them to suit their liking.

@benmosher
Copy link
Member

benmosher commented Aug 2, 2016

@jfmengels I think AVA and Mocha have an easier time detecting test files on account of having global identifiers to key off of (i.e. describe, at least, for Mocha).

And dev deps transcend tests. I use a bunch of them in my gulpfile.js which is conspicuously above my /src.

I agree that it would be ideal that ESLint handle it, but a regex- or glob-flavored devDependencies setting is not really very complicated relative to its practical usefulness.

@jfmengels
Copy link
Collaborator

I think AVA and Mocha have an easier time detecting test files on account of having global identifiers to key off of (i.e. describe, at least, for Mocha).

Some Mocha rules check the use of before and other hooks. If you know you are in a test file, you can safely assume that that identifier refers to Mocha's hook. But if you don't know (which is currently the case), you have to be more cautious of what you report (check if you're in a describe etc.). AVA is more explicit as it requires to import AVA in the file, but you need the same kind of checks.
I just meant to say, that they would benefit quite a lot of having a well-configured glob configuration. But I'm wandering off.

that's what I do now, but that's not something I can specify in a shared config :-/

Yeah, I get the pain of that. I don't know if this was discussed in the ESLint issue, but I hope that the recommended configurations from plugins and configs can specify globs. That should definitely be possible.

Anyway, go ahead and do this if you want, I just hope that not many other rules will get similar requests.

@gunnx
Copy link

gunnx commented Aug 11, 2016

In the case when you wish to have your .spec.js file alongside implementation then to pass in an ignore regex would be useful. Otherwise it would be back to a separate test folder with an override .eslintrc

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

No branches or pull requests

4 participants