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

CLI: Add --exclude-checks support #318

Merged
merged 10 commits into from
Nov 7, 2023
Merged

CLI: Add --exclude-checks support #318

merged 10 commits into from
Nov 7, 2023

Conversation

swissspidy
Copy link
Member

@swissspidy swissspidy commented Nov 6, 2023

Fixes #308

Successfully tested using something like this:

wp plugin check web-stories # Returns some errors
wp plugin check web-stories --exclude-checks=late_escaping,plugin_readme,plugin_review_phpcs # No errors

@swissspidy swissspidy added [Type] Enhancement A suggestion for improvement of an existing feature WP-CLI Issues related to WP-CLI labels Nov 6, 2023
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@swissspidy Almost LGTM, only I think we should choose a consistent name for the new argument.

@@ -62,6 +62,10 @@ public function __construct( Plugin_Context $plugin_context ) {
* [--checks=<checks>]
* : Only runs checks provided as an argument in comma-separated values, e.g. i18n_usage, late_escaping. Otherwise runs all checks.
*
* [--exclude-checks=<checks>]
Copy link
Member

@felixarntz felixarntz Nov 6, 2023

Choose a reason for hiding this comment

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

You call this ignore-checks / ignore-slugs everywhere else in this PR. I don't feel strongly about either way, but we should go with a consistent term throughout. It makes sense that the parameter uses "checks" while the code uses "slugs", but I mean we should only use the term "ignore" or "exclude" throughout.

I personally prefer exclude-checks I think, but either way WFM.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense, let's settle on exclude

includes/Checker/Abstract_Check_Runner.php Outdated Show resolved Hide resolved
@swissspidy swissspidy changed the title CLI: Add —exclude-checks support CLI: Add --exclude-checks support Nov 6, 2023
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

Thanks @swissspidy, LGTM!

@swissspidy
Copy link
Member Author

@adamsilverstein Just as an FYI, I'll update the GitHub Action right after merging this one.

Copy link
Member

@adamsilverstein adamsilverstein left a comment

Choose a reason for hiding this comment

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

Fantastic!

@mukeshpanchal27 mukeshpanchal27 merged commit b7cac5d into trunk Nov 7, 2023
11 checks passed
@mukeshpanchal27 mukeshpanchal27 deleted the fix/308-exclude branch November 7, 2023 04:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement A suggestion for improvement of an existing feature WP-CLI Issues related to WP-CLI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLI: make it easier to exclude specific checks
4 participants