Update houndci to support es6 #3326

enejb · 2021-03-31T19:00:36Z

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 · 2021-03-31T19:04:02Z

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

dcalhoun

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.

dcalhoun · 2021-03-31T20:12:48Z

.hound.yml

+eslint:
+  enabled: true


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

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

hound

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)

hound

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 · 2021-03-31T21:36:43Z

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 · 2021-03-31T21:51:10Z

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 · 2021-04-01T16:24:09Z

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 · 2021-04-01T20:49:07Z

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 · 2021-04-01T23:32:46Z

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 · 2021-04-02T17:20:15Z

Since we disabled Hound I am closing this PR.

dannyrb · 2021-04-08T01:36:22Z

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

enejb added 3 commits March 31, 2021 11:59

Update houndci to support es6

d903865

Update file to be in json format

dc69728

Reorder imports

ab05abd

enejb requested a review from guarani March 31, 2021 19:06

enejb self-assigned this Mar 31, 2021

enejb requested a review from ceyhun March 31, 2021 19:13

enejb added [Type] Enhancement Improves a current area of the editor dependencies Pull requests that update a dependency file labels Mar 31, 2021

Fixes to the json document.

1e19909

dcalhoun reviewed Mar 31, 2021

View reviewed changes

Lets see if this will make hound fail

39c1c07

hound bot reviewed Mar 31, 2021

View reviewed changes

enejb added 2 commits March 31, 2021 14:00

Add eslint ignore to hound

908f177

Add swiftlint config to hound

a0d2c4b

hound bot reviewed Mar 31, 2021

View reviewed changes

enejb closed this Apr 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update houndci to support es6 #3326

Update houndci to support es6 #3326

enejb commented Mar 31, 2021 •

edited

Loading

peril-wordpress-mobile bot commented Mar 31, 2021 •

edited

Loading

dcalhoun left a comment •

edited

Loading

dcalhoun Mar 31, 2021

enejb Mar 31, 2021

hound bot left a comment

hound bot left a comment

enejb commented Mar 31, 2021

dcalhoun commented Mar 31, 2021

enejb commented Apr 1, 2021

dcalhoun commented Apr 1, 2021

enejb commented Apr 1, 2021

enejb commented Apr 2, 2021

dannyrb commented Apr 8, 2021

Update houndci to support es6 #3326

Update houndci to support es6 #3326

Conversation

enejb commented Mar 31, 2021 • edited Loading

peril-wordpress-mobile bot commented Mar 31, 2021 • edited Loading

dcalhoun left a comment • edited Loading

Choose a reason for hiding this comment

dcalhoun Mar 31, 2021

Choose a reason for hiding this comment

enejb Mar 31, 2021

Choose a reason for hiding this comment

hound bot left a comment

Choose a reason for hiding this comment

hound bot left a comment

Choose a reason for hiding this comment

enejb commented Mar 31, 2021

dcalhoun commented Mar 31, 2021

enejb commented Apr 1, 2021

dcalhoun commented Apr 1, 2021

enejb commented Apr 1, 2021

enejb commented Apr 2, 2021

dannyrb commented Apr 8, 2021

enejb commented Mar 31, 2021 •

edited

Loading

peril-wordpress-mobile bot commented Mar 31, 2021 •

edited

Loading

dcalhoun left a comment •

edited

Loading