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

Set up linting and style checking for javascript #1924

Closed
jfly opened this issue Sep 10, 2017 · 5 comments · Fixed by #5259
Closed

Set up linting and style checking for javascript #1924

jfly opened this issue Sep 10, 2017 · 5 comments · Fixed by #5259

Comments

@jfly
Copy link
Contributor

jfly commented Sep 10, 2017

It's important that this work on both js and jsx files. It would be particularly awesome if it also works on javascript inside of html and javascript inside of erb, but it's okay if that's not easy to do.

(See #1856 (comment) for some discussion about indentation of promises)

@jfly jfly mentioned this issue Sep 10, 2017
2 tasks
@jfly
Copy link
Contributor Author

jfly commented Sep 10, 2017

I know that pre-commit does some linting of javascript for us, but I think it's only configured to run on js files, and it might not be aware of es2016+ stuff that we're doing now.

@UnsolvedCypher
Copy link
Contributor

I think it might make sense to switch to something like ESlint which does a very good job of enforcing both code style and catching anti-patterns. Maybe we could add the ESlint and a config and then add it as a pre-commit once all of the initial errors have been worked through?

@viroulep
Copy link
Contributor

Since #4381 we actually use the overcommit gem which runs jshint on our codebase before commits!
However, while I've seen some errors being reported on the js files I wrote, I haven't seen some on jsx.
Overcommit seems to have something ready for ESlint, I'd definitely be interested to see what it has to say about our code.
Is that something you would like to try integrating?

@UnsolvedCypher
Copy link
Contributor

I'd be interested in trying to integrate this if the WST thinks it's a good direction to go in.

I ran ESlint on the JavaScript code to see what kind of errors it would get. For the most part:

  • functions used before they are defined
  • strings not being consistent in quotes used (single vs double)
  • incorrect indentation
  • incorrect spacing (such as missing between if and (
  • use of var instead of let or let instead of const
  • line length > 100

Over 75% of the issues raised can be automatically fixed by ESlint. I'm sure some of the other ones are false positives that would need to be fixed by further configuration.

@viroulep
Copy link
Contributor

viroulep commented Mar 4, 2020

Yes, if we can run this over the whole code base (js + jsx) that would be very nice to ensure we have a consistent coding style over the whole code base.
Would you be able to submit a draft PR with the appropriate settings for overcommit (if you did look into it, or just how you ran it if you ran/configured it manually), as well as the automatic fixes?
This way we could have a place to discuss if we are happy with eslint reports and also how we can handle the other errors raised!

Thanks for looking into this!

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 a pull request may close this issue.

3 participants