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

Add basic tooling for JS linting #3167

Merged
merged 6 commits into from
May 20, 2019
Merged

Conversation

aitbw
Copy link
Contributor

@aitbw aitbw commented Apr 9, 2019

Description

See #2849 for further discussion prior to this PR

This PR aims to add basic tooling for JS linting based on @airbnb's JavaScript Style Guide by doing the following:

  • Adds a package.json file with the bare minimum to work as expected —including a Yarn task to automatically check if the rules for all JS files on Solidus (minus those under guides, sandbox, node_modules and vendor) are being followed. The dependencies for this to work are based on this NPM package, which doesn't include linting specific to JS frameworks, such as React

  • Ignores unnecessary newly-introduced files and folders (I'm looking at you, node_modules 👀)

  • Adds ESLint config file with all the warnings/errors raised when running yarn lint disabled

  • Adds ESLint inline config comments on several files under backend, as yarn lint was complaining even with the config file

  • Enable JS linting for Hound based on the newly-introduced .eslintrc.json file

  • Whitelist Solidus-specific global variables

This should allow for other future PRs to re-enable rules and fix its related issues.

Let me know what you think! 🙌

Checklist:

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have updated Guides and README accordingly to this change (if needed)
  • I have added tests to cover this change (if needed)

Copy link

@houndci-bot houndci-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some files could not be reviewed due to errors:

�[31mERROR�[0m: Can't find config file: airbnb-base
�[31mERROR�[0m: Can't find config file: airbnb-base

Copy link

@houndci-bot houndci-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some files could not be reviewed due to errors:

�[31mERROR�[0m: Can't find config file: airbnb-base
�[31mERROR�[0m: Can't find config file: airbnb-base

Copy link

@houndci-bot houndci-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some files could not be reviewed due to errors:

�[31mERROR�[0m: Can't find config file: airbnb-base
�[31mERROR�[0m: Can't find config file: airbnb-base

Copy link

@houndci-bot houndci-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some files could not be reviewed due to errors:

�[31mERROR�[0m: Can't find config file: airbnb-base
�[31mERROR�[0m: Can't find config file: airbnb-base

@aitbw aitbw force-pushed the aitbw/js-linting branch 2 times, most recently from e61d599 to 7d30172 Compare April 12, 2019 12:11
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Thanks!

@kennyadsl
Copy link
Member

But I think it should not close #2849, since it also expects other PR to re-introduce disabled rules.

@aitbw
Copy link
Contributor Author

aitbw commented Apr 15, 2019

Correct, @kennyadsl —let me edit the description to avoid automatic closure of that issue.

Copy link
Contributor

@JuanCrg90 JuanCrg90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really great. I have some suggestions regarding our globals

.eslintrc.json Outdated Show resolved Hide resolved
.eslintrc.json Outdated Show resolved Hide resolved
@aitbw
Copy link
Contributor Author

aitbw commented May 20, 2019

Added a new commit to comply with your suggestions, @tvdeyen 🙌 let me know what you think!

Copy link
Contributor

@jacobherrington jacobherrington left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this, we should merge it pending @tvdeyen's approval.

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome. Thanks 👏

@tvdeyen tvdeyen merged commit f017429 into solidusio:master May 20, 2019
@aitbw aitbw deleted the aitbw/js-linting branch May 20, 2019 12:33
@aitbw aitbw mentioned this pull request May 22, 2019
4 tasks
@aitbw aitbw mentioned this pull request Jun 11, 2019
2 tasks
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 this pull request may close these issues.

6 participants