-
Notifications
You must be signed in to change notification settings - Fork 60
Add: code linting scripts #196
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great and much needed, thanks so much for adding it!
Some notes:
- WordPress scripts was really out of date, so
npm iwas generating a lot of warnings, some severe. I updated usingnpm audit fix --forceand committed those changes here: b113980, I hope that's ok! lint:cssworks :check:lint:phpworks ✅ but requires composer locally, we can add some docs to as a follow up herelint:jsalso works but reports a number of errors for me:
/Users/jffng/Documents/A8C/create-block-theme/admin/js/embed-local-font.js
21:21 error 'FileReader' is not defined no-undef
26:20 error 'Font' is not defined no-undef
31:18 error 'evt' is already declared in the upper scope on line 12 column 29 no-shadow
34:10 error 'font' is already declared in the upper scope on line 26 column 9 no-shadow
/Users/jffng/Documents/A8C/create-block-theme/admin/js/form-script.js
1:10 error 'toggleForm' is defined but never used no-unused-vars
/Users/jffng/Documents/A8C/create-block-theme/admin/js/google-fonts.js
24:16 error Identifier 'get_google_fonts' is not in camel case camelcase
92:23 error 'FontFace' is not defined no-undef
99:20 error Identifier 'loaded_face' is not in camel case camelcase
103:5 error Unexpected console statement no-console
/Users/jffng/Documents/A8C/create-block-theme/src/font-family.js
1:1 error 'react' should be listed in the project's dependencies. Run 'npm i -S react' to add it import/no-extraneous-dependencies
/Users/jffng/Documents/A8C/create-block-theme/src/manage-fonts.js
1:1 error 'react' should be listed in the project's dependencies. Run 'npm i -S react' to add it import/no-extraneous-dependencies
4:2 error Usage of `__experimentalConfirmDialog` from `@wordpress/components` is not allowed.
See https://developer.wordpress.org/block-editor/contributors/develop/coding-guidelines/#experimental-and-unstable-apis for details @wordpress/no-unsafe-wp-apis
107:25 error Expected '===' and instead saw '==' eqeqeq
177:8 error Translations should not contain collapsible whitespace (consecutive spaces) @wordpress/i18n-no-collapsible-whitespace
177:8 error Translate function arguments must be string literals @wordpress/i18n-no-variables
184:8 error Translate function arguments must be string literals @wordpress/i18n-no-variables
/Users/jffng/Documents/A8C/create-block-theme/update-google-fonts-json-file.js
19:3 error Unexpected console statement no-console
38:4 error Unexpected console statement no-console
40:4 error Unexpected console statement no-console
43:3 error Unexpected console statement no-console
/Users/jffng/Documents/A8C/create-block-theme/update-version-and-changelog.js
48:7 error Parsing error: Unexpected reserved word 'package'. (48:7)
Do you see something different?
Thanks for working on this! I appreciate this change.
Could we implement the linter so that it is applied only to the files changed in a branch?
This way, we could add this progressively to the repo files and reduce the error surface and potential conflicts with the ongoing work (active branches/PRs).
WordPress scripts was really out of date, so npm i was generating a lot of warnings, some severe. I updated using npm audit fix --force and committed those changes here: b113980, I hope that's ok!
Makes sense, thanks for that!
Do you see something different?
Yes, I'm seeing the same results for lint:js. I don't think these can be fixed automatically, so these just need fixing manually. I can run them as part of this PR, so everything is formatted.
Could we implement the linter so that it is applied only to the files changed in a branch?
Yes, we could do. So this PR would just merge the tools for linting, but wouldn't commit any of the newly formatted files? And maybe we could add a commit hook to trigger the linter on commit?
Yes, we could do. So this PR would just merge the tools for linting, but wouldn't commit any of the newly formatted files? And maybe we could add a commit hook to trigger the linter on commit?
Exactly! this sounds like a nice solution.
# Conflicts: # css/manage-fonts.css # src/font-face.js # src/font-family.js # src/manage-fonts.js
Thanks for your input! I've updated this with...
- Removed all formatting changes, so these can be done when the files themselves are updated in different PRs
- Added a few more config files for editor settings: eslint, stylelint, browserlist
- Installed husky and added a pre-commit hook that runs the linters on staged files, and will prevent a commit if there are any errors
This should be ready for another review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is getting better and better!
I have only 2 small comments:
- If I run the JS linter, the code is formatted, not following the WordPress style. See this screenshot for example (left is before linter, right is after). The most notorious change is the removal of the spaces in inside braces :
myFunc ( parameter );->myFunc(parameter);.
- Could we add the file/setting to add the globals that are marked as missing by the linter?
If I run the JS linter, the code is formatted, not following the WordPress style.
Oh good spot! I think this was caused by using the standard prettier package rather than wp-prettier, which I've just learned exists 😅 I'm seeing the spaces inside brackets when I run npm run lint:js:fix now (and also a lot more errors..)
Could we add the file/setting to add the globals that are marked as missing by the linter?
Do you mean when the linter flags a package as missing? Could you give me an example of one of the linter errors?
Do you mean when the linter flags a package as missing? Could you give me an example of one of the linter errors?
Yep! I mean some objects that are part of the global scope and, for that reason, not imported from anywhere. They are throwing errors:
21:21 error 'FileReader' is not defined no-undef
26:20 error 'Font' is not defined no-undef
There are not many, so maybe we can fix that with comments ad-hoc too.
Ah thanks, that's helpful! I've added some additional ESLint rules so these globals should now be ignored.
# Conflicts: # package-lock.json # package.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good to me, I left two small comments.
Should we add build to this also?
Yes, makes sense! I've made the prettier and stylelint ignore files the same, so they both now include the build, vendor, and node_modules directories.
If I run npm run lint:js:fix should it fix all the auto-fixable problems in the JS files of the repo right?
I'm doing that and I'm not getting any changes on the JS files. Only 1 problem was reported.
I think this was working on a previous revision of this branch.
PHP and CSS fix commands are working as expected.
Hmm, looks like the custom ESLint rules were overwriting too many rules perhaps. I've removed them and replaced them with one that disables the no-undef rule, which is the one that was picking up on the global definitions. Hopefully this runs correctly!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working nicely now.
Let's merge this! 🚀
Uh oh!
There was an error while loading. Please reload this page.
This adds a couple of scripts and dependencies to find linting errors following the WordPress coding standards:
lint:css&lint:css:fixlint:js&lint:js:fixlint:php&lint:php:fixIt also adds a pre-commit hook to run the linter on staged files before they're commited.
The setup is based on the tools in Gutenberg.