voronianski / flux-comparison

:pencil: Practical comparison of different Flux solutions
https://labs.voronianski.dev/flux-comparison/
2.79k stars 214 forks source link

How to lint the code? #59

Closed gaearon closed 8 years ago

gaearon commented 8 years ago

The contributing guide suggests using ESLint, and there's a .eslintrc in the project root, but eslint is not in devDependencies. After adding it there and running it, I got (literally) over 9000 violations:

screen shot 2015-09-29 at 20 43 20

Should we remove the section on using ESLint from Contributing, as well as .eslintrc? It seems to do more harm than good at this point. :-)

voronianski commented 8 years ago

@gaearon I'm using eslint validator in Sublime Text 3 and it currently shows no errors at all..

gaearon commented 8 years ago

Hmm. You're using global eslint then, right? Maybe 1.x doesn't work but 0.x works?

voronianski commented 8 years ago

@gaearon I've updated eslint globally and see some indent errors now, but I'm a bit stuck with it:

/Users/dmitri/github/flux-comparison/redux/js/reducers/products.js
  11:9   error  Expected indentation of 4 space characters but found 8   indent
  12:13  error  Expected indentation of 8 space characters but found 12  indent
  15:13  error  Expected indentation of 8 space characters but found 12  indent
  17:9   error  Expected indentation of 4 space characters but found 8   indent
  18:13  error  Expected indentation of 8 space characters but found 12  indent
  19:13  error  Expected indentation of 8 space characters but found 12  indent
  21:9   error  Expected indentation of 4 space characters but found 8   indent
  22:13  error  Expected indentation of 8 space characters but found 12  indent

What's wrong with that? Is the rule "indent": [2, 4], is not correct?

gaearon commented 8 years ago

Usually, with switch, the cases shouldn't be indented. (At least, that's a common style I saw.)

should be:

switch (stuff) {
case STUFF: // <---- not indented
  stuff();
  break;
}
voronianski commented 8 years ago

@gaearon hmm yeah, strange..

gaearon commented 8 years ago

Not really strange—that's a very common style. It's also very popular in Java / C# / C code.

gaearon commented 8 years ago

I think the justification for this is that case is like a label—not a block.

voronianski commented 8 years ago

@gaearon I've just committed an option to allow using indent in switches - https://github.com/voronianski/flux-comparison/commit/0b6a39055df60187ad8014cb681f18ffb5d5c422#diff-1dc6ee56b778cd91e0327b52aaeaa1b9R13

voronianski commented 8 years ago

@gaearon so after that linting with CLI is fine:

eslint redux/js

/Users/dmitri/github/flux-comparison/redux/js/actions/ActionCreators.js
   5:23  warning  "getState" is defined but never used  no-unused-vars
  31:23  warning  "getState" is defined but never used  no-unused-vars

✖ 2 problems (0 errors, 2 warnings)
voronianski commented 8 years ago

@gaearon the problem with eslint CLI is that if running eslint redux/ it will also lint files in build folder which, of course, contains a lot of errors.

gaearon commented 8 years ago

I'm still getting a few more issues. I'll send a PR fixing those.

voronianski commented 8 years ago

@gaearon thanks

gaearon commented 8 years ago

https://github.com/voronianski/flux-comparison/pull/61