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 jscs & jshint with eslint #554

Merged
merged 3 commits into from
Mar 15, 2016
Merged

replace jscs & jshint with eslint #554

merged 3 commits into from
Mar 15, 2016

Conversation

alexlamsl
Copy link
Collaborator

@XhmikosR as per request, this is the migration to eslint.

As I'm completely new to this module, .eslintrc.yml is generated using eslint --init and feeding it the source files of this project, plus a few minor edits.

@alexlamsl alexlamsl mentioned this pull request Mar 15, 2016
@XhmikosR
Copy link
Collaborator

Great work! Only issue is with the format you went with for eslintrc. I'm used to the json. Also, are you sure the recommended ones don't conflict with the custom rules you added?

Generally, I just specify what I need, but if we can make this cleaner I'm all for it as we keep things simple. Having to go through all of those rules is a p.i.t.a. :/

@alexlamsl
Copy link
Collaborator Author

Ah okay. I run into trouble and take note from Node.js and went with yaml, and all of those are generated by eslint --init. I only editted the offending rules but didn't add any new ones.

Let me start again with JSON with just extends and env and see if that'd fare better...

@alexlamsl
Copy link
Collaborator Author

@XhmikosR right, 3rd try at this... forced push and with very minimal .eslintrc.json this time.

@@ -0,0 +1,17 @@
{
"env": {
"browser": true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd suggest that we specify the environment where needed and not globally.

@XhmikosR
Copy link
Collaborator

@alexlamsl: apart from my comment, I'd go with .eslintrc and merge if everything passes. We can tweak the config later if needed.

@alexlamsl
Copy link
Collaborator Author

@XhmikosR according to here, they claim .eslintrc to be deprecated

But if you insist, your wish is my command 👼

@XhmikosR
Copy link
Collaborator

No worries, it's a minor thing. 👍

@alexlamsl
Copy link
Collaborator Author

Turns out what I meant was shared-node-browser - the common subset of both...

@alexlamsl
Copy link
Collaborator Author

Hmm, I'm confused - the test passed on my machine but not on Travis...

@alexlamsl
Copy link
Collaborator Author

So I'll chalk that up as node-chakracore-specific behaviour...

alexlamsl added a commit that referenced this pull request Mar 15, 2016
replace jscs & jshint with eslint
@alexlamsl alexlamsl merged commit 8090803 into kangax:gh-pages Mar 15, 2016
@kangax
Copy link
Owner

kangax commented Mar 15, 2016

👍

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