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

Replace jshint with eslint #11915

Merged
merged 8 commits into from
Jul 19, 2019
Merged

Replace jshint with eslint #11915

merged 8 commits into from
Jul 19, 2019

Conversation

simison
Copy link
Member

@simison simison commented Apr 4, 2019

Replace jshint with eslint. Just a quick stint, mostly deleted files and plugged some overrides to eslint config to see if this would be easy or complicated.

Complications come mostly from the fact that ES5 and ES6 files are mixed up in different folders, specifically _inc which has both.

Moving old JS files out from _inc would probably make this much easier.

Resolves #11914

Changes proposed in this Pull Request:

Added a ES5 eslint config file which is used for linting both modules/** and _inc/*(! not **) files, also fixed some linting errors in few files.
Might be a good idea to switch some of the rules from warn to error and fix these issues too.

TODO:

  • Use migration ruleset from @wordpress/eslint-plugin
  • Ensure eslint overrides paths are correct
  • remove inline jshint rules (e.g.) - in followup PR
  • Configure correct globals - in followup PR

Testing instructions:

yarn lint or yarn lint-es6 && yarn lint-es5

Proposed changelog entry for your changes:

  • N/A

@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello simison! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D26470-code before merging this PR. Thank you!

@jetpackbot
Copy link
Collaborator

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: April 29, 2019.
Scheduled code freeze: April 22, 2019

Generated by 🚫 dangerJS against 57cf0d3

@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello simison! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D29957-code before merging this PR. Thank you!

@jetpackbot
Copy link
Collaborator

jetpackbot commented Jun 28, 2019

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: August 6, 2019.
Scheduled code freeze: July 30, 2019

Generated by 🚫 dangerJS against c1bd239

@matticbot
Copy link
Contributor

simison, Your synced wpcom patch D29957-code has been updated.

1 similar comment
@matticbot
Copy link
Contributor

simison, Your synced wpcom patch D29957-code has been updated.

@brbrr brbrr added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Jul 17, 2019
@matticbot
Copy link
Contributor

simison, Your synced wpcom patch D29957-code has been updated.

1 similar comment
@matticbot
Copy link
Contributor

simison, Your synced wpcom patch D29957-code has been updated.

@brbrr brbrr added this to the 7.6 milestone Jul 18, 2019
@matticbot
Copy link
Contributor

simison, Your synced wpcom patch D29957-code has been updated.

Copy link
Member

@jeherve jeherve 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 it. I think it would be nice to merge it as is, and then fix all the current es6 and es5 warnings in a follow-up PR, and then update the rules to match WordPress', so we don't add more warnings in the future:
https://github.com/WordPress/gutenberg/blob/4905299edfb1f652229daea695fe196b55d0bf76/packages/eslint-plugin/configs/jshint.js

This is going to need a rebase first though. I took care of that. Will merge when the tests pass.

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Jul 19, 2019
@jeherve jeherve merged commit a5e8a83 into master Jul 19, 2019
@jeherve jeherve deleted the remove/jshint branch July 19, 2019 11:35
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Jul 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove jshint
5 participants