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

Including JS linting in the Solidus workflow #2849

Closed
jacobherrington opened this issue Sep 19, 2018 · 8 comments
Closed

Including JS linting in the Solidus workflow #2849

jacobherrington opened this issue Sep 19, 2018 · 8 comments
Labels
type:enhancement Proposed or newly added feature

Comments

@jacobherrington
Copy link
Contributor

After making #2847 and hearing @tvdeyen's feedback, I feel like we need to adopt some JS standards for Solidus.

Solidus isn't a JS project; it doesn't seem like this is something we should obsess over. However the project relies on having a functioning admin UI, so it's pragmatic to make sure we have high code quality when it comes to JS.

I'm not sure what the best solution for Solidus is (and don't want to start an argument), but I think it's worth discussing with other contributors. Thoughts?

@tvdeyen
Copy link
Member

tvdeyen commented Sep 19, 2018

👍

Repeating my comment from #2847

Being too strict with linters can harm contributions. We want only the bare minimum of style linting as possible.

We should:

  1. Set up JS linting
  2. Disable all checks
  3. Enable one check
  4. Fix the issues
  5. Repeat from 3

That's how we did it with Rubocop and this is how we should do this with JS linting.

@ericsaupe
Copy link
Contributor

From what I've seen, the AirBnB JS style guide seems to be pretty well received. Not saying we have to follow it exactly, just like we have exceptions in our Rubocop linter, but may be a good start.

https://github.com/airbnb/javascript

@jacobherrington
Copy link
Contributor Author

I agree, but @tvdeyen makes a great point about adopting it incrementally.

@dhonig
Copy link

dhonig commented Sep 26, 2018

@ericsaupe @tvdeyen I've been using the AirBNB style guide for JS and React on projects outside of Solidus when no better alternative is already provided. I've found it to be pretty sensible in most cases though some devs have argued to relax certain points. I think its important to stress the importance of using good tooling to make working with ESLint pain free. For example both VS.code and Webstorm have very good plugins that automate working with ESLint, maybe even make it fun....So lets make sure we mention tooling in the docs.

@tvdeyen
Copy link
Member

tvdeyen commented Sep 26, 2018

I use the AirBnB lint rules as well. But we need to relax even more, because our current code will violate it all the time.

👍 for strong tooling. We still need to ensure to not scare contributors

@jacobherrington jacobherrington added type:enhancement Proposed or newly added feature good first issue labels Nov 21, 2018
@kennyadsl
Copy link
Member

I'm also 👍 with what has been proposed by @tvdeyen in #2849 (comment). AirBnB rules (more relaxed) are fine but that's the goal we should strive step by step as proposed.

@seand7565
Copy link
Contributor

#3167, #3212, and #3225 all were merged - bringing in JS linting and introducing more rules. Just wanted to check on the status here - are there more JS linting rules we want to implement, or is this issue good to close?

@kennyadsl
Copy link
Member

Yes, I think we are fine now, thanks @seand7565!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:enhancement Proposed or newly added feature
Projects
None yet
Development

No branches or pull requests

6 participants