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

move to eslint #1508

Closed
gordonwoodhull opened this issue Dec 9, 2018 · 9 comments
Closed

move to eslint #1508

gordonwoodhull opened this issue Dec 9, 2018 · 9 comments

Comments

@gordonwoodhull
Copy link
Contributor

JSCS was merged into ESLint a couple of years ago, and then hit end of life.

Although it's still working for us, it's a source of security warnings from NPM because it uses an old lodash etc. Probably ESLint has some nice newer features which will be handy. And we could probably get rid of JSHint, making our linting faster.

@gordonwoodhull
Copy link
Contributor Author

gordonwoodhull commented Dec 9, 2018

I migrated the .jscsrc using polyjuice, and ESLint reports 734 errors, but all of them are indenting or jsdoc errors. So we need to loosen the indentation rules (because we follow recommended D3 indentation thanks to @kum-deepak) and we probably need to loosen the jsdoc rules as well, or add a lot of return and parameter descriptions.

@gordonwoodhull
Copy link
Contributor Author

Migrating jshint is a little easier. I tried eslint-rules-mapper and it produced a much larger .eslintrc, and 918 errors. However, all of them were due to the migration tool adding rules we don't want.

@gordonwoodhull
Copy link
Contributor Author

gordonwoodhull commented Dec 9, 2018

For now I'm adding .eslintrc.jscs and .eslintrc.jshint to develop, to be merged later.

Mostly I couldn't figure out how to use grunt-eslint correctly - even though it clearly is reading my .eslintrc, it reports a whole bunch of new errors.

Here is how I attempted to change Gruntfile.js:

diff --git a/Gruntfile.js b/Gruntfile.js
index d3d220f0..0b5ac66e 100644
--- a/Gruntfile.js
+++ b/Gruntfile.js
@@ -82,7 +82,7 @@ module.exports = function (grunt) {
                 }
             }
         },
-        jscs: {
+        eslint: {
             source: {
                 src: [
                     '<%= conf.src %>/**/*.js',
@@ -91,9 +91,9 @@ module.exports = function (grunt) {
                     'Gruntfile.js',
                     'grunt/*.js',
                     '<%= conf.web %>/stock.js'],
-                options: {
-                    config: '.jscsrc'
-                }
+            },
+            options: {
+                configFile: '.eslintrc'
             }
         },
         jshint: {

@gordonwoodhull
Copy link
Contributor Author

Aha, polyjuice can also convert .jshintrc, so we now have a decent start at .eslintrc

Also, it looks like the extra errors when running in grunt are simply because we also check spec/, which used to have looser rules - how did we do that?

@kum-deepak
Copy link
Collaborator

If nothing else works, we can switch to what Angular does - use two different configs

@gordonwoodhull
Copy link
Contributor Author

Definitely. I thought this is what we have for our jshint/jscs configs, but now I can't find any trace of it. I think we started linting the specs in #975.

@kum-deepak
Copy link
Collaborator

You have excellent memory of all issues 😄

@gordonwoodhull
Copy link
Contributor Author

Haha, I've just gotten really good at searching the repo, and the unkindly-named git blame, which is a great history tool. Amazing how much I have learned (and forgotten) maintaining this library. 😉

@gordonwoodhull
Copy link
Contributor Author

It turns out the specs simply override rules using comment directives - so instead of jshint/jscs overrides, there are a few less eslint overrides now.

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

No branches or pull requests

2 participants