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

Update houndci to support es6 #3326

Closed
wants to merge 7 commits into from
Closed

Update houndci to support es6 #3326

wants to merge 7 commits into from

Conversation

enejb
Copy link
Contributor

@enejb enejb commented Mar 31, 2021

Updates hound ci to support es6.

Currently, hound ci doesn't seem to support es6. See #2582

I am hoping this PR properly configures hound ci so that when it runs it validates es6 syntext as expected.

To test:
Does hound CI work as expected?

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered if this change warrants user-facing release notes more info and have added them to RELEASE-NOTES.txt if necessary.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Mar 31, 2021

Wanna run full suite of Android and iOS UI tests? Click here and 'Approve' CI job!

@enejb enejb requested a review from guarani March 31, 2021 19:06
@enejb enejb self-assigned this Mar 31, 2021
@enejb enejb requested a review from ceyhun March 31, 2021 19:13
@enejb enejb added [Type] Enhancement Improves a current area of the editor dependencies Pull requests that update a dependency file labels Mar 31, 2021
Copy link
Member

@dcalhoun dcalhoun left a comment

Choose a reason for hiding this comment

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

I'm not highly familiar with HoundCI configuration, but overall this approach seems sound to me.

We currently only support JSON or YAML formats for .eslintrc since we parse and merge configs.

I was puzzled by the move from .eslintrc.js to .eslintrc.json, but after finding the related documentation and issues (houndci/hound#1630, houndci/hound#1793) it makes sense now.

Currently, hound ci doesn't seem to support es6. See WordPress/gutenberg#29670

Was the above from your PR description intended to link to #2582 (review)? The current link didn't appear to include any references to Hound.

Comment on lines +3 to +4
eslint:
enabled: true
Copy link
Member

Choose a reason for hiding this comment

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

This Hound documentation would imply we need to add a pointer to our own eslint configuration file. Did you find Hound worked without explicitly adding the path?

Suggested change
eslint:
enabled: true
eslint:
enabled: true
config_file: .eslintrc.json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't say I am able to confirm either way.

Copy link

@hound hound 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:

Cannot find module '@wordpress/eslint-plugin'
Cannot find module '@wordpress/eslint-plugin'
Referenced from: .eslintrc
Error: Cannot find module '@wordpress/eslint-plugin'
    at ModuleResolver.resolve (/home/linters/app/versions/eslint-4.19.1/node_modules/eslint/lib/util/module-resolver.js:74:19)
    at resolve (/home/linters/app/versions/eslint-4.19.1/node_modules/eslint/lib/config/config-file.js:470:32)
    at load (/home/linters/app/versions/eslint-4.19.1/node_modules/eslint/lib/config/config-file.js:551:26)
    at configExtends.reduceRight (/home/linters/app/versions/eslint-4.19.1/node_modules/eslint/lib/config/config-file.js:425:36)
    at Array.reduceRight ()
    at applyExtends (/home/linters/app/versions/eslint-4.19.1/node_modules/eslint/lib/config/config-file.js:403:26)
    at loadFromDisk (/home/linters/app/versions/eslint-4.19.1/node_modules/eslint/lib/config/config-file.js:523:22)
    at Object.load (/home/linters/app/versions/eslint-4.19.1/node_modules/eslint/lib/config/config-file.js:559:20)
    at Config.getLocalConfigHierarchy (/home/linters/app/versions/eslint-4.19.1/node_modules/eslint/lib/config.js:227:44)
    at Config.getConfigHierarchy (/home/linters/app/versions/eslint-4.19.1/node_modules/eslint/lib/config.js:179:43)

Copy link

@hound hound 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:

Cannot find module '@wordpress/eslint-plugin'
Cannot find module '@wordpress/eslint-plugin'
Referenced from: .eslintrc
Error: Cannot find module '@wordpress/eslint-plugin'
    at ModuleResolver.resolve (/home/linters/app/versions/eslint-4.19.1/node_modules/eslint/lib/util/module-resolver.js:74:19)
    at resolve (/home/linters/app/versions/eslint-4.19.1/node_modules/eslint/lib/config/config-file.js:470:32)
    at load (/home/linters/app/versions/eslint-4.19.1/node_modules/eslint/lib/config/config-file.js:551:26)
    at configExtends.reduceRight (/home/linters/app/versions/eslint-4.19.1/node_modules/eslint/lib/config/config-file.js:425:36)
    at Array.reduceRight ()
    at applyExtends (/home/linters/app/versions/eslint-4.19.1/node_modules/eslint/lib/config/config-file.js:403:26)
    at loadFromDisk (/home/linters/app/versions/eslint-4.19.1/node_modules/eslint/lib/config/config-file.js:523:22)
    at Object.load (/home/linters/app/versions/eslint-4.19.1/node_modules/eslint/lib/config/config-file.js:559:20)
    at Config.getLocalConfigHierarchy (/home/linters/app/versions/eslint-4.19.1/node_modules/eslint/lib/config.js:227:44)
    at Config.getConfigHierarchy (/home/linters/app/versions/eslint-4.19.1/node_modules/eslint/lib/config.js:179:43)

@enejb
Copy link
Contributor Author

enejb commented Mar 31, 2021

It looks like hound currently doesn't support plugins. So we can't have the same eslint config as before.
See houndci/hound#1838

@dcalhoun
Copy link
Member

It looks like hound currently doesn't support plugins. So we can't have the same eslint config as before.
See houndci/hound#1838

@enejb bummer. So, what do you think next steps of exploration are? I am a bit confused as to the state of things.

I suppose prior to 39c1c07 Hound was relying upon the default, built-in ESLint config. I suppose the alternative path forward could be to revert 39c1c07 and rely upon that? The downside would be that Hound wouldn't really be checking our code against the ESLint rules we use. It would, presumably, unblock your issue though, correct?

@enejb
Copy link
Contributor Author

enejb commented Apr 1, 2021

I think we should be more explicit about the rules that we want to check. It is a bummer that we can't have the plugins that we want but I think having a eslint config that we use specifically for Hound would help us control that better vs using the default.

I still wasn't able to replace the error that I am seeing in #2582.
I was hoping that ab05abd would trigger it.

Do you understand what the difference between hound and the "Check Correctness" tests that seems to run eslint . --ext .js command are?

How are they different? If they serve the same purpose maybe we can disable hound for js files?
Since to me they seem to be taken care of by Check Correctness already?

@dcalhoun
Copy link
Member

dcalhoun commented Apr 1, 2021

I think we should be more explicit about the rules that we want to check. It is a bummer that we can't have the plugins that we want but I think having a eslint config that we use specifically for Hound would help us control that better vs using the default.

@enejb agreed. I'm just hesitant to maintain a duplicate ESLint config to mimic @wordpress/eslint-plugin. I fear that it would become outdated quickly.

I still wasn't able to replace the error that I am seeing in #2582.
I was hoping that ab05abd would trigger it.

My thought is that the original import error doesn't show up currently because that after 39c1c07 Hound is now throwing the plugin error prior to hitting the ES6 import error.

Prior to 39c1c07, my thought is that the error no longer showed up because your PR included Hound configuration to enable ESLint, which included the default ESLint config that enabled ES6.

Do you understand what the difference between hound and the "Check Correctness" tests that seems to run eslint . --ext .js command are?

How are they different? If they serve the same purpose maybe we can disable hound for js files?
Since to me they seem to be taken care of by Check Correctness already?

No, I don't fully understand why we would have both Hound and "Check Correctness" for JavaScript. Given Hound's lack of support for ESLint plugins, I wonder if the shortest and best path forward is to disable checking JavaScript with Hound. That would leave "Check Correctness" in place that leverages @wordpress/eslint-plugin.

Thoughts?

@enejb
Copy link
Contributor Author

enejb commented Apr 1, 2021

My vote would be to disable it for now.
But I don't really have a good understanding of the differences.
maybe @ceyhun, @cameronvoell, and @hypest might have a better idea on the next steps here.

@enejb
Copy link
Contributor Author

enejb commented Apr 2, 2021

Since we disabled Hound I am closing this PR.

@enejb enejb closed this Apr 2, 2021
@dannyrb
Copy link

dannyrb commented Apr 8, 2021

FWIW, the hound maintainers have been fairly responsive when contacted. Its mostly a matter of them "allow listing" plugins

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file [Type] Enhancement Improves a current area of the editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants