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

Feature/update ignore filter message #30

Merged
merged 6 commits into from
Apr 10, 2016
Merged

Feature/update ignore filter message #30

merged 6 commits into from
Apr 10, 2016

Conversation

jonathansamines
Copy link
Contributor

When the eslint@2 upgrade was made, the filterIgnoredFileMessages function was broken, since the new eslint@2 changed the message produced for the ignored files.

Maybe somehow related to the reported error on ember-cli-eslint#44.

@jonathanKingston
Copy link
Member

Hey! Thanks for this, it's been a while since I wrote that so makes sense it was broken.

It looks like they have added the ability to ignore those by option now:
https://github.com/eslint/eslint/blob/a35f1274cc553789e7a83d8cf501c82be7b6b342/lib/cli-engine.js#L662

This should mean that: https://github.com/jonathanKingston/broccoli-lint-eslint/blob/03a6d3f4934b11f7ac2d90c663c822dd372cf16f/lib/index.js#L71 can be changed to have a default of ignore: true if the key isn't defined.

Either way there is only one line change here in this pull: https://github.com/jonathanKingston/broccoli-lint-eslint/pull/30/files if we are going with this option then this should be cleaned.

If possible a test would be brilliant!

@jonathansamines
Copy link
Contributor Author

I guess the option is for either ignore the .eslintignore patterns or not. Since if the option is false then the ignoreResult won´t be created and the lint warning will be created instead. I'll try to add some tests.

@jonathanKingston
Copy link
Member

Ah yeah, I think that was there before actually thanks!

@jonathansamines
Copy link
Contributor Author

@jonathanKingston I just added tests cases. Let me know what do you think.

@BrianSipple BrianSipple added this to the 2.2.0 milestone Apr 6, 2016
@BrianSipple
Copy link
Collaborator

@jonathansamines pinging for a heads-up about a few conflicts after merging #25.

@jonathansamines
Copy link
Contributor Author

@BrianSipple thanks for pinging. I solved the merging conflicts.

@@ -51,10 +51,11 @@ function filterIgnoredFileMessages(errors) {
function filterAllIgnoredFileMessages(result) {
const resultOutput = result;

result.results.map((resultItem) => {
result.results.forEach((resultItem) => {
resultItem.messages = filterIgnoredFileMessages(resultItem.messages);
return resultItem;
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like this is just a leftover from using map, but with forEach, the return actually becomes unnecessary 😄.

Everything else looks good to go, though!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Completely true, removed ;)

@BrianSipple BrianSipple merged commit e669e39 into ember-cli:master Apr 10, 2016
@BrianSipple
Copy link
Collaborator

@jonathansamines Thanks!

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

Successfully merging this pull request may close these issues.

3 participants