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

Replace jshint by eslint #450

Merged
merged 11 commits into from
Apr 7, 2017
Merged

Conversation

elboletaire
Copy link
Collaborator

@elboletaire elboletaire commented Apr 7, 2017

I'm creating the PR as a WIP because I'm currently working on it and I see things which create some questions for me:

  • I get a lot of no-undef errors due to the injected libraries (under testing, the it and describe, more specifically). I've currently switch the rule to show warnings instead of errors, but I think it's better to disable it adding /* eslint no-undef: 'off' */ for each test file that brings an error. If you're ok with it, I'll do a commit re-enabling the rule and setting the rule file by file.
  • I've set no-console to warn. Would you like to set it to off, or is it ok to stay as warnings?

I'll also try to edit this post to add some notes about some decisions I made:

  • The first commit is just the automatically created file from eslint --init. There are a lot of rules set to off that you may want to check.
  • I've set no-extra-boolean-cast to warn in order to change from errors to warnings, so you don't forget about it :p
  • I've set no-empty with "allowEmptyCatch": true, to allow empty catch statements.
  • As there's no option for setting the .eslintignore file as a grunt-eslint parameter, I've decided to move both .eslintrc.json and .eslintignore to the root path, and remove the tasks/support folder, which was containing the .jshintrc file.
  • We should consider changing the strict rules, as they're not needed for nodejs modules (they're in strict mode by default).

refs #436

@matteofigus
Copy link
Member

I've read your mind before so opened #449 💃

@elboletaire
Copy link
Collaborator Author

haha did not see it :p

Then I'll merge my branch with master when your PR is accepted. I remove that question from the PR, but please, try to answer the other ones too 😃

@elboletaire
Copy link
Collaborator Author

Added note:

  • As there's no option for setting the .eslintignore file as a grunt-eslint parameter, I've decided to move both .eslintrc.json and .eslintignore to the root path, and remove the tasks/support folder, which was containing the .jshintrc file.

@elboletaire elboletaire changed the title [WIP] Add eslint Add eslint Apr 7, 2017
@elboletaire
Copy link
Collaborator Author

elboletaire commented Apr 7, 2017

I've removed the WIP because my work is done until you answer that two questions in the PR 😃

Tests run perfect locally, but for some reason weren't executed in travis nor in appveyor.

@elboletaire
Copy link
Collaborator Author

PS. Merging with master, as #449 has been merged

@elboletaire
Copy link
Collaborator Author

Added:

  • We should consider changing the strict rules, as they're not needed for nodejs modules (they're in strict mode by default).

@elboletaire elboletaire changed the title Add eslint [WIP] Add eslint Apr 7, 2017
@elboletaire elboletaire changed the title [WIP] Add eslint Add eslint Apr 7, 2017
@elboletaire
Copy link
Collaborator Author

Merged with master and fixed no-unused-vars issues appeared after enabling the rule again.

Note that I've set no-unused-vars: 'off' in registry-domain-components-cache.js because there's giving a false positive (apparently does not properly detect the scope of the response var).

@elboletaire elboletaire changed the title Add eslint Replace jshint by eslint Apr 7, 2017
@elboletaire elboletaire mentioned this pull request Apr 7, 2017
@matteofigus
Copy link
Member

This looks good to me for now. Let's create separate issues for each rule that we may want to change. Agree about the no-strict, in case let's make a different PR for that.

Thanks for working on this!

@matteofigus matteofigus merged commit 0575a4a into opencomponents:master Apr 7, 2017
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.

2 participants