feat(config): Deprecate JSHint in favor of ESLint #1097

feimosi · 2015-12-10T20:03:18Z

This is the basic ESLint setup extending well-known Airbnb code style and dedicated Angular rules.
What also has changed:

removed eslint task from client.views path in gulp watch, possibly we can think of using some html linter
changed most of the globals by environment property, still I'd recommend not using / limit the use of global variables by dependency injection
the previous contributor's rules are left in place, but we need to review them to make the migration seamless. For now it's not consistent with JSHint, so someone with longer experiences in the project could help convert all the rules - docs are quite well-written
I haven't used Grunt much, so even though it was a basic change, someone could take a closer look :)

References #1072

mleanos · 2015-12-11T04:04:43Z

One thing to keep in mind is that we've decided on using John Papa's Angular Style-guide. I haven't taken a close look at this PR, but does the ESLint configuration take this into consideration? Or are there conflicting styles?

#943

feimosi · 2015-12-11T21:53:42Z

ESLint itself has only general purpose JS linting, but the plugin I've added has the ability to lint Angular apps. As they write in the summary: "some rules defined in John Papa's Guideline have been implemented". I just don't know how the defaults follow your conventions.

codydaig · 2015-12-12T16:14:32Z

@feimosi Thanks for the PR!

I am in favor of ESLINT, however, it doesn't match our style guide currently. There are a lot of linting errors, and the handful that I picked out and looked at I do not agree with.

Ex.)
In creating an object, it should not have a trailing comma
Incorrect: { foo: 'bar', }
Correct: { foo: 'bar',}

Ex.)
Strict mode is not permitted.
We want to have 'use strict' enabled.

Can you go through the errors and see if we can adjust the linter accordingly. If there are actual issues somewhere, fix those in a separate PR to make this PR pass the tests.

Thanks!

feimosi · 2015-12-13T15:01:36Z

@codydaig Ok, sure, I'll go through them when I find some time.

rhutchison · 2015-12-29T04:50:07Z

@codydaig, I am confused by this - typo?

Ex.)
In creating an object, it should not have a trailing comma
Incorrect: { foo: 'bar', }
Correct: { foo: 'bar',}

rhutchison · 2015-12-29T04:57:24Z

@feimosi I am in favor of this PR, but have questions about maintaining a single eslintrc file for this project.

I noticed that you have added the reference to angular default config, but I am curious what you have noticed when running these rules against the server-side files. Also, I have not seen much progress with adopting the style guide - are these rules failing?

Should nodejs lint rules be separated from angularjs rules? Can they be the same and still be restrictive enough?

feimosi · 2015-12-29T10:26:04Z

@rhutchison sorry, I wasn't available lately to take care of this PR.
Regarding the Angular config, I haven't noticed any conflicts so far. It looks like those rules are only fired for Angular-specific code. ESLint allows you to specify separate configs for each folder, but unfortunately not for filename patterns, nonetheless it's not a problem for now.
So I think we can keep both rule sets together.

ilanbiala · 2015-12-31T21:01:02Z

@feimosi still have lots of errors in the build. Can you take a look at them?

ilanbiala · 2016-02-11T01:54:37Z

package.json

@@ -66,6 +66,9 @@
  },
  "devDependencies": {
    "coveralls": "^2.11.4",
+    "eslint-config-airbnb": "^2.0.0",


These should be ~

ilanbiala · 2016-02-11T01:54:52Z

@feimosi can you rebase and fix up some of the changes requested above so we can merge?

feimosi · 2016-02-11T16:52:15Z

@ilanbiala I need some more time for this PR. Too busy these days.
Regarding angular-plugin, eventually I decided to remove it as I've noticed, in same cases it indeed interferes with node's code.

trendzetter · 2016-02-14T21:41:51Z

I had too disable eslint and rely only on jshint because it gave me too many warnings on code I migrated. The --fix option in the command line eslint is also broken so I couldn't fix the thousands of indentation errors which were cluttering my screen.
How do you guys correct styling errors automatically?

ilanbiala · 2016-02-15T00:33:29Z

@trendzetter I don't think anyone has corrected errors automatically.

feimosi · 2016-02-15T21:58:38Z

@trendzetter Last time I tried autocorrect it was working pretty badly, so I wouldn't rely on it.
@ilanbiala The config is ready. Now someone needs to review it. There are still ~150 errors, but I think they should be fixed and not muted by the rules. Also I had to turn off several rules following JSHint config, but I really think you should prepare a more strict style guide.

ilanbiala · 2016-02-15T22:03:12Z

@feimosi there shouldn't be any errors. JSHint never had any, so we should be able to switch to ESLint with whatever properties there and still not have any errors.

feimosi · 2016-02-15T22:09:27Z

@ilanbiala The linting rules are already very loose and there are many inconsistencies throughout the codebase. If I mute the given errors, the sense of using ESLint slowly fades out.

trendzetter · 2016-02-17T19:58:50Z

I could reduce the indentation and other styling errors for code I imported from close to 2000 to 10 using js-beautify
Then I fixed the remaining remarks manually. I am now running eslint with gulp again.

lirantal · 2016-02-28T19:05:34Z

It's quite obvious that this PR should bring with it actual fixes to all the errors and warnings so that eslint can actually go in.
@feimosi are you going to take care of it?

lirantal · 2016-02-28T19:06:43Z

@ilanbiala we can figure out which rules trigger errors and discuss on whether we should 'drop the rule' to make it compatible with what we have today or adopt that rule and make the necessary changes.
What do you think of this plan?

ilanbiala · 2016-02-28T19:42:09Z

Yeah, that sounds fine. First we need to rebase and go over each type of error we currently get. Though I'd rather rebase because some code will change and the errors may disappear/more may appear.

feimosi · 2016-02-29T22:31:41Z

That was exactly my point. ESLint introduces much more new rules, so they should be discussed by the core team and decided whether to keep them or not.

ilanbiala · 2016-03-01T03:42:25Z

@feimosi can you point us to the new ones/ones we don't use specifically? Also, can you rebase before we go any further?

feimosi · 2016-03-01T09:12:32Z

@ilanbiala These rules are failing:

comma-dangle
comma-spacing
consistent-return
default-case
guard-for-in
key-spacing
keyword-spacing
new-cap
no-alert
no-spaced-func
no-throw-literal
no-trailing-spaces
no-undef
no-unneeded-ternary
no-unreachable
one-var
one-var-declaration-per-line
quote-props
spaced-comment
warnings)
wrap-iife

You can always check it with this command:
eslint modules/**/*.js | awk 'NF>1{print $NF}' | sort | uniq

ilanbiala · 2016-03-13T17:37:18Z

@lirantal @mleanos final check before I merge?

lirantal · 2016-03-14T06:34:19Z

I'm all up for it.

lirantal · 2016-03-14T06:37:28Z

Just to add though that there are warnings we still have to deal with, such as:

ilanbiala · 2016-03-14T14:10:00Z

@lirantal I think that's fine that we have console statements and putting the comment above seems like clutter, no?

ilanbiala · 2016-03-14T22:55:57Z

@feimosi can you rebase and squash?

lirantal · 2016-03-15T05:12:30Z

@ilanbiala yeah not the end of the world.

feimosi · 2016-03-15T15:36:31Z

Done, you'd better hurry with that merge. It starts to get boring fixing the merge conflicts all the time.

ilanbiala · 2016-03-15T15:58:11Z

@feimosi there are some build errors, can you take a look?

ilanbiala · 2016-03-15T16:01:42Z

package.json

@@ -68,6 +68,8 @@
  },
  "devDependencies": {
    "coveralls": "~2.11.6",
+    "eslint": "~2.2.0",


Please delete JSHint dependencies too and remove .jshintrc.

Add basic ESLint setup extending well-known Airbnb code style. Fixes meanjs#1072, meanjs#1097

feimosi · 2016-03-15T18:36:17Z

@ilanbiala everything's done

mleanos · 2016-03-15T19:39:05Z

I just restarted the build. Looks like a connectivity/time-out error.

ilanbiala · 2016-03-15T20:06:43Z

LGTM, merging. @feimosi thanks for the hard work and following up on this every time!

feat(config): Deprecate JSHint in favor of ESLint

mleanos · 2016-03-15T20:28:20Z

@feimosi Yes, thank you for all the hard work! No more complex/unnecessary ternary's, among other styling issues you fixed here ;)

feimosi · 2016-03-15T21:42:07Z

Thanks too, I'm glad I could help! :)

codydaig · 2016-03-17T21:15:16Z

WooHoo!!

Add basic ESLint setup extending well-known Airbnb code style. Fixes meanjs#1072, meanjs#1097

trendzetter · 2016-04-22T07:59:57Z

I am not happy, has any of you written any amount of code with this enabled? It's really unbearable. It should be possible to turn this strict cosmetic mode off when you are not writing code for the meanjs framework itself. If you are writing application logic this is just too distracting, you just want to get something moving first and then brush it up.
How do I turn off specific eslint rules easily?
Why are rules like http://eslint.org/docs/rules/no-console enabled? The node app is intentionally writing to the console so this rule will be triggered all the time.
Plus it contains at least 2 bugs (#1319) probably not tested well.

simison · 2016-04-22T09:47:26Z

config/env/development.js

      //    fileName: 'access-%DATE%.log', // if rotating logs are active, this fileName setting will be used
      //    frequency: 'daily',
      //    verbose: false
      //  }
-      //}
+      // }


That extra space breaks indentation here, now space to the next level is 1 space. Was it required by eslint? Should the whole block be moved by one space instead?

trendzetter · 2016-04-22T19:51:12Z

These lint errors are making me angry, sorry for the ranting. I found I can skip rules by setting them them 0 in .eslintrc rules. I am giving it another shot at cleaning things up with the best beautification tricks found so far.

trendzetter · 2016-04-22T20:34:25Z

Just tried the eslint cli again and it has improved in recent times. It now is able to fix some issues automatically using the --fix flag. You would expect it to fix more issues automatically but at least its a beginning.

codydaig added the enhance:feature label Dec 12, 2015

lirantal added this to the 0.5.0 milestone Dec 16, 2015

ilanbiala reviewed Feb 11, 2016
View reviewed changes

feimosi force-pushed the eslint-integration branch from dc678ad to c642da7 Compare February 15, 2016 21:51

lirantal self-assigned this Feb 28, 2016

ilanbiala closed this Mar 1, 2016

ilanbiala reopened this Mar 1, 2016

feimosi force-pushed the eslint-integration branch from c642da7 to 858f98b Compare March 1, 2016 08:53

feimosi force-pushed the eslint-integration branch from 8f3562d to 62d3e91 Compare March 15, 2016 15:34

ilanbiala reviewed Mar 15, 2016
View reviewed changes

feimosi force-pushed the eslint-integration branch from 62d3e91 to 717470c Compare March 15, 2016 18:08

feimosi force-pushed the eslint-integration branch from 717470c to 349f16d Compare March 15, 2016 18:10

feat(config): Deprecate JSHint in favor of ESLint

d14d513

Add basic ESLint setup extending well-known Airbnb code style. Fixes meanjs#1072, meanjs#1097

feimosi force-pushed the eslint-integration branch from 349f16d to d14d513 Compare March 15, 2016 18:11

ilanbiala added a commit that referenced this pull request Mar 15, 2016

Merge pull request #1097 from feimosi/eslint-integration

b0e6b4e

feat(config): Deprecate JSHint in favor of ESLint

ilanbiala merged commit b0e6b4e into meanjs:master Mar 15, 2016

HarryGogonis pushed a commit to HarryGogonis/mean that referenced this pull request Mar 18, 2016

feat(config): Deprecate JSHint in favor of ESLint

7a7ba23

Add basic ESLint setup extending well-known Airbnb code style. Fixes meanjs#1072, meanjs#1097

simison reviewed Apr 22, 2016
View reviewed changes

feat(config): Deprecate JSHint in favor of ESLint #1097

feat(config): Deprecate JSHint in favor of ESLint #1097

Conversation

feimosi commented Dec 10, 2015

mleanos commented Dec 11, 2015

feimosi commented Dec 11, 2015

codydaig commented Dec 12, 2015

feimosi commented Dec 13, 2015

rhutchison commented Dec 29, 2015

rhutchison commented Dec 29, 2015

feimosi commented Dec 29, 2015

ilanbiala commented Dec 31, 2015

ilanbiala Feb 11, 2016

Choose a reason for hiding this comment

ilanbiala commented Feb 11, 2016

feimosi commented Feb 11, 2016

trendzetter commented Feb 14, 2016

ilanbiala commented Feb 15, 2016

feimosi commented Feb 15, 2016

ilanbiala commented Feb 15, 2016

feimosi commented Feb 15, 2016

trendzetter commented Feb 17, 2016

lirantal commented Feb 28, 2016

lirantal commented Feb 28, 2016

ilanbiala commented Feb 28, 2016

feimosi commented Feb 29, 2016

ilanbiala commented Mar 1, 2016

feimosi commented Mar 1, 2016

ilanbiala commented Mar 13, 2016

lirantal commented Mar 14, 2016

lirantal commented Mar 14, 2016

ilanbiala commented Mar 14, 2016

ilanbiala commented Mar 14, 2016

lirantal commented Mar 15, 2016

feimosi commented Mar 15, 2016

ilanbiala commented Mar 15, 2016

ilanbiala Mar 15, 2016

Choose a reason for hiding this comment

feimosi commented Mar 15, 2016

mleanos commented Mar 15, 2016

ilanbiala commented Mar 15, 2016

mleanos commented Mar 15, 2016

feimosi commented Mar 15, 2016

codydaig commented Mar 17, 2016

trendzetter commented Apr 22, 2016 • edited Loading

simison Apr 22, 2016 • edited Loading

Choose a reason for hiding this comment

trendzetter commented Apr 22, 2016

trendzetter commented Apr 22, 2016

trendzetter commented Apr 22, 2016 •

edited

Loading

simison Apr 22, 2016 •

edited

Loading