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 JSHint ESLint config #12840

Merged
merged 2 commits into from
Dec 19, 2018
Merged

Add JSHint ESLint config #12840

merged 2 commits into from
Dec 19, 2018

Conversation

ntwb
Copy link
Member

@ntwb ntwb commented Dec 13, 2018

Description

This pull request is a follow up to #12763 and seeks to add a new jshint ESLint shared configuration of the existing WordPress .jshintrc file.

This pull request makes available a "drop-in" ESLint JSHInt configuration that will allow WordPress to easily transition away from JSHint to ESLint per the #WP31823 ticket whilst the significant legacy JavaScript is updated to adhere to the WordPress JavaScript Coding Standards in the @wordpress/eslint-plugin/recommended ESLint configuration.

How has this been tested?

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@ntwb ntwb added [Type] Build Tooling Issues or PRs related to build tooling [Tool] ESLint plugin /packages/eslint-plugin labels Dec 13, 2018
@ntwb ntwb requested review from gziolo and aduth December 13, 2018 13:02
@youknowriad
Copy link
Contributor

Not certain how I feel about adding this to the package, if it's for legacy purpose, I propose using it in WordPress directly in addition to the eslint package, instead of adding it to the reusable package

@ntwb
Copy link
Member Author

ntwb commented Dec 14, 2018

I included it in the package as for any existing plugin, theme, or core that is looking to update their build tooling to switch from JSHint to ESLint can do so using a backwards compatible set of rules.

The rules themselves in the configuration are not all similarly named so without some knowledge of JSHint and ESLint making such a ruleset configuration would not be a trivial task. This same approach would be required to have to first disable all the unrequired rules included in @wordpress/eslint-plugin/recommended to then add the rules I've proposed in this PR.

I fully plan to use this ruleset myself in the plugins I'm associated with whilst the teams get familiar with ESLint work can begin on migrating to the full @wordpress/eslint-plugin/recommended configuration as time permits.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

I think we should clarify when jshint config should be used. Otherwise I think the explanation @ntwb shared makes a lot of sense. We can remove this config when introducing next breaking change as it looks like a temporary addition.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Code wise looks good to me. It just needs the decision whether we want to have it included. I am personally fine with having it introduced.

@gziolo gziolo added the Needs Decision Needs a decision to be actionable or relevant label Dec 17, 2018
@youknowriad
Copy link
Contributor

You all convinced me, I'm fine to include now. We can remove it later if needed.

@gziolo gziolo removed the Needs Decision Needs a decision to be actionable or relevant label Dec 19, 2018
@gziolo gziolo added this to the 4.8 milestone Dec 19, 2018
@gziolo gziolo merged commit 0484bc3 into master Dec 19, 2018
@gziolo gziolo deleted the eslint-jshint-config branch December 19, 2018 08:26
youknowriad pushed a commit that referenced this pull request Jan 9, 2019
* Add ESLint JSHint config

* Update packages/eslint-plugin/README.md

Co-Authored-By: ntwb <[email protected]>
youknowriad pushed a commit that referenced this pull request Jan 9, 2019
* Add ESLint JSHint config

* Update packages/eslint-plugin/README.md

Co-Authored-By: ntwb <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Tool] ESLint plugin /packages/eslint-plugin [Type] Build Tooling Issues or PRs related to build tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants