-
Notifications
You must be signed in to change notification settings - Fork 310
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
Code Quality: Update codes to pass php lint #201
Conversation
composer.json
Outdated
"squizlabs/php_codesniffer": "^3.6", | ||
"wp-coding-standards/wpcs": "^2.3" | ||
}, | ||
"scripts": { | ||
"lint": "./vendor/bin/phpcs --standard=phpcs.xml" | ||
"lint": "phpcs --standard=phpcs.xml" |
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.
Probably a Windows-specific problem, the following error occurred:
'.' is not recognized as an internal or external command,
operable program or batch file.
Script ./vendor/bin/phpcs --standard=phpcs.xml handling the lint event returned with error code 1
I changed the way to specify the path according to gutenberg repository.
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 command doesn't work for me anymore:
[InvalidArgumentException]
Script "phpcs" is not defined in this package
Let's revert this for now and come back to it in a secondary PR where we can come up with a good cross-platform approach.
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.
Thank you for taking the time to put this together!
Just a couple of notes/comments but overall looks great :)
09-code-data-basics-esnext/index.php
Outdated
plugins_url( 'style.css', __FILE__ ) | ||
plugins_url( 'style.css', __FILE__ ), | ||
null, | ||
get_bloginfo( 'version' ), |
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.
Can we reuse the $asset_file['version']
here? My concern is that this will be cached until the version of WP changes.
composer.json
Outdated
"squizlabs/php_codesniffer": "^3.6", | ||
"wp-coding-standards/wpcs": "^2.3" | ||
}, | ||
"scripts": { | ||
"lint": "./vendor/bin/phpcs --standard=phpcs.xml" | ||
"lint": "phpcs --standard=phpcs.xml" |
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 command doesn't work for me anymore:
[InvalidArgumentException]
Script "phpcs" is not defined in this package
Let's revert this for now and come back to it in a secondary PR where we can come up with a good cross-platform approach.
plugin-sidebar/plugin-sidebar.php
Outdated
plugins_url( 'plugin-sidebar.css', __FILE__ ) | ||
plugins_url( 'plugin-sidebar.css', __FILE__ ), | ||
null, | ||
get_bloginfo( 'version' ), |
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.
Same concern about the asset version being tied to the WP version.
Thank you for reviewing the code ! |
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.
LGTM 👍 - thanks again for putting this together!
I have noticed many errors in the php lint.
This PR updates codes to pass correctly based on what phpcs pointed out.
Mainly, I have addressed the following:
add_action
to after function to eliminate missing doc error.@package
tag$in_footer
) ofwp_register_script
$ver
) ofwp_register_style
node_modules
,block.asset.php
)require_once
instead ofinclude
As this is my first PR, please do not hesitate to point out any problems.