-
-
Notifications
You must be signed in to change notification settings - Fork 490
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 ValidTaxonomySlugSniff #2485
base: develop
Are you sure you want to change the base?
Conversation
- Adds `AbstractValidSlugSniff`, which can serve as a base for sniffs that check a function parameter to ensure it is a valid slug. - Refactors `ValidPostTypeSlugSniff` to extend `AbstractValidSlugSniff`. - Introduces `ValidTaxonomySlugSniff`, which also extends `AbstractValidSlugSniff`. - Adds `WPReservedKeywordHelper` to centrally store reserved keywords. All classes inherit the public property `$exclude` from `AbstractFunctionRestrictionsSniff`. `WPReservedKeywordHelper::$terms` requires a "Last updated for WordPress x.x."
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.
Hi @IanDelMar, welcome to WPCS and thank you for your willingness to contribute.
It is generally considered a good idea to first open an issue to discuss whether a potential new sniff is desired and what to take into account, instead of straight away creating a PR (and refactoring a whole lot of pre-existing code).
In this case, the PR, in part, solves an existing, open issue - #871 -, though I get the impression you didn't see or read that issue. Am I right ?
Having said that, I think the principle of this new sniff is a good idea and the PR is a reasonable first pass (which is not unexpected as the PR is basically reusing pre-existing code with some select search-and-replace).
I've left a lot of comments inline for you to consider.
Other than that, neither the abstract nor the new Helper class have code coverage as there are no @covers
tags refering to these. This needs to be fixed (as can also be seen by the nearly 2% drop in code coverage as reported by CI).
You may also want to consider updating the commit message/PR description to actually describe what problem you are trying to solve with this new sniff.
The implementation details can be seen from the code. The use case of why this sniff is a good idea to begin with and what information/documentation/handbook rules the sniff is based on and who would benefit from this sniff should be argued in the PR description.
@@ -0,0 +1,70 @@ | |||
<?php |
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.
Please make the code samples more realistic. You've clearly done a search-and-replace on the test files of the ValidPostTypeSlug
sniff, but the function signature of register_taxonomy( string $taxonomy, array|string $object_type, array|string $args = array())
is not the same as that of register_post_type( string $post_type, array|string $args = array() )
, which means that the fast majority of code samples used in the tests are actually invalid function calls.
Now, some invalid function calls are to be expected in sniff tests (to stress-test the sniff), but the "normal" tests, should be valid 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.
Of course
Co-authored-by: Juliette <[email protected]>
Co-authored-by: Juliette <[email protected]>
Co-authored-by: Juliette <[email protected]>
Co-authored-by: Juliette <[email protected]>
Co-authored-by: Juliette <[email protected]>
Co-authored-by: Juliette <[email protected]>
Co-authored-by: Juliette <[email protected]>
@jrfnl Thank you for the detailed feedback.
Understood. I apologise if my actions came across as rude. May I suggest adding this information to the
No, you are not. I am, of course, willing to go through all the comments and make the necessary changes to the code. However, based on this comment, I feel this PR may not be the best place to address the valid slug issue for taxonomies. My intention was to share the code already implemented in the |
Co-authored-by: Juliette <[email protected]>
Co-authored-by: Juliette <[email protected]>
Co-authored-by: Juliette <[email protected]>
AbstractValidSlugSniff
, which can serve as a base for sniffs that check a function parameter to ensure it is a valid slug.ValidPostTypeSlugSniff
to extendAbstractValidSlugSniff
.ValidTaxonomySlugSniff
, which also extendsAbstractValidSlugSniff
.WPReservedKeywordHelper
to centrally store reserved keywords.All sniff classes inherit the public property
$exclude
fromAbstractFunctionRestrictionsSniff
.WPReservedKeywordHelper::$terms
requires a "Last updated for WordPress x.x."