Closed adhamhf closed 6 years ago
Hey @adhamhf, I spent some time verifying that broccoli-lint-eslint
correctly detects and uses an .eslintignore
file, and it seems to be working fine.
I did have a thought while working on it, though; it's possible that the ignore pattern is not working correctly because of the way that Ember changes the directory structure during it's build phase. As far as I understand it, I'm pretty sure that the tests
directory is merged with the app
directory by Broccoli before our ESLint plugin is run. What that means is that ESLint doesn't see a tests/reports/
directory, it just sees a reports/
directory. This is probably the reason that the ignore file is not working right.
Unfortunately, this is really confusing behavior but there isn't anything that we can really do to fix it, since that merging is done upstream where we can't control it. However, if you add a file like this:
// .eslintignore
**/*.js
inside tests/reports
, ESLint should ignore all of those files, even when Broccoli changes the directory structure.
Thanks @alexlafroscia. Unfortunately this didn't work either....
We are using the ember-cli-code-coverage addon and putting the reports in tests/reports.
I create the .eslintignore file as directed, but this file is still being linted tests/reports/lcov-report/prettify.js
.
The directory is dynamically generated by ember-cli-code-coverage, but as a test I put the same .eslintignore file in that directory and still the file was linted. Weird.
For now I have switched back to the default location for these report files which is coverage
that works around the problem for now.
Hmm... Interesting.
So tests/reports/.eslintignore
with the contents I put above didn't solve things for you? That's a shame, but I'm glad you found a workaround for the time being.
I'll make a fresh Ember app with that code coverage addon and ember-cli-eslint
and see if I can reproduce the issue locally, and if I can get a fix working.
Thanks for the additional information!
@adhamhf Are you working with QUnit or Mocha? Any other addons that might be relevant? I just want to make sure that my local environment while testing is as close as possible to what you're using.
@alexlafroscia Qunit and here are my dependencies:
"devDependancies": { "bower": "^1.7.9",
"broccoli-asset-rev": "^2.4.4",
"ember-a11y-testing": "^0.1.3",
"ember-ajax": "^2.4.1",
"ember-cli": "^2.6.2",
"ember-cli-app-version": "^1.0.0",
"ember-cli-babel": "^5.1.6",
"ember-cli-code-coverage": "^0.2.2",
"ember-cli-dependency-checker": "^1.3.0",
"ember-cli-eslint": "^1.6.0",
"ember-cli-htmlbars": "^1.0.8",
"ember-cli-htmlbars-inline-precompile": "^0.3.2",
"ember-cli-inject-live-reload": "^1.4.0",
"ember-cli-mirage": "0.2.0-beta.9",
"ember-cli-moment-shim": "2.0.0",
"ember-cli-postcss": "^3.0.0",
"ember-cli-qunit": "^2.1.0",
"ember-cli-release": "^0.2.9",
"ember-cli-sass": "^5.3.1",
"ember-cli-sri": "^2.1.0",
"ember-cli-template-lint": "^0.4.11",
"ember-cli-uglify": "^1.2.0",
"ember-concurrency": "0.7.8",
"ember-data": "^2.6.1",
"ember-devtools": "4.4.5",
"ember-export-application-global": "^1.0.5",
"ember-intl": "^2.12.4",
"ember-load-initializers": "^0.5.1",
"ember-pikaday": "2.0.0",
"ember-resolver": "^2.0.3",
"ember-sinon": "0.5.1",
"ember-sinon-qunit": "",
"ember-test-selectors": "0.0.3",
"ember-wormhole": "0.4.0",
"jsonwebtoken": "^7.1.7",
"loader.js": "^4.0.10",
"phantomjs-prebuilt": "^2.1.7"
},
"dependencies": {
"ncp": "^2.0.0",
"path": "^0.12.7",
"request-promise": "^3.0.0",
"tar.gz": "^1.0.5",
"lodash": "^4.13.1"
}
Thanks!
Alright, I finally got some time to investigate this a bit further. It seems like the issue does have something to do with the way that ember-cli
merges the app/
and tests/
directories. I tried a lot of things and only finally had success by giving broccoli-lint-eslint
an explicit path to the .eslintignore
file, and then writing patterns in that file relative to the combined app/
+ tests/
directory.
@BrianSipple @Turbo87 Do you have any ideas on how we can make this more user-friendly? Ideally, a "normal" .eslintignore
file should "just work" but that might be impossible to reconcile with the fact that Broccoli modifies the directory structure before performing the ESLint evaluation.
I made a demo repo that reproduces the error here:
https://github.com/alexlafroscia/ember-cli-eslint-ignore-test-app
If you run
eslint app tests
there are no issues, but the linting fails with
ember test
I've been playing around in the demo app, @alexlafroscia, but so far to no avail. And I tried a number of approaches:
.eslintignore
to try and ignore test/report
.eslintignore
file at just about every directory leveloptions
hash that broccoli-cli-eslint
sends through to eslint
right after our other settings in the lintTree
hook.Buts here’s the other thing. The last experiment did lead to being able to verify that the .eslintignore
file is being read, as running ember test
when overriding the configFile
option would throw an error when I specified a file path that didn’t exist.
So that’s one fact we can isolate.
I’m curious to see how this behaves with ESLint 3.x — and whether or not this isn’t just a problem with 2.x — but I didn’t get that far into the process at the moment.
FWIW - I'm not able to get ember-cli-eslint v3.0.0 to respect an .eslintignore file at all.
I have an .eslintignore file at the root of my project, and no matter what I set it to (including **/*.js
) I cannot get it to ignore the files under tests.
Hey gents, any updates on this? Do we wanna at least call this out on the readme as I feel it'll be a good heads up when trying to decide if one should switch over from the default jshint.
@raikoroje no updates afaik but we'd love to get a PR if you want to work on it
I have a PR into broccoli-lint-eslint that I believe resolves this issue: https://github.com/ember-cli/broccoli-lint-eslint/pull/85
@LucasHill thanks! your patch has been released as broccoli-lint-eslint@3.2.1
and should be used by ember-cli-eslint
automatically.
can anyone verify that this did indeed fix things?
Its working in our project, I had to drop and re-add ember-cli-eslint to make yarn choose the new build. I hope to hear it fixes others as well. Its worth noting there is still some existing confusing behavior. For example if you ignore a file (that possibly would fail the linter), it'll still show up as a "pass eslint' in the qunit tests. I was thinking of making that less confusing in a separate PR and show no test if it was ignored.
I was thinking of making that less confusing in a separate PR and show no test if it was ignored.
that should actually be a relatively easy change from what I can tell
@Turbo87 Here is my stab at that: https://github.com/ember-cli/broccoli-lint-eslint/pull/86
Is this still an issue?
I'm assuming this is fixed by https://github.com/ember-cli/broccoli-lint-eslint/pull/86, if not please let me know and we can reopen this issue
It's not picking up the file paths again, not sure why:
/mirage/test-data/
/mirage/test-data/**
mirage/test-data/**
/mirage/test-data/v1/base/
mirage/test-data/v1/base/style.js
Doesn't seem like any combination here works! Please reopen.
I'm on "ember-cli-eslint": "^4.2.3",
Broken for me as well. "ember-cli-eslint": "^4.2.3"
I have an .eslintignore file in the root of my project. It contains:
/tests/reports/*
The goal of this is to get all the istanbul reports ignored. If I just run
eslint
this works, but if I runember test
the .eslintignore file is not respected. Additionally I have tired added a tests/.eslintignore file but it too appears to be ignored when running test. I don't think its a problem with my syntax since runningeslint
manually respects the .eslintignore file.