Skip to content

Add support for severity levels for warnings and errors#527

Merged
swissspidy merged 13 commits intotrunkfrom
479-add-severity-level
Aug 14, 2024
Merged

Add support for severity levels for warnings and errors#527
swissspidy merged 13 commits intotrunkfrom
479-add-severity-level

Conversation

@ernilambar
Copy link
Copy Markdown
Member

Fixes #479

  • Default severity level for check is 5
  • --severity argument is added for wp plugin check

Example:

# Show errors and warnings with severity greater or equal to 7
wp plugin check foo-sample --severity=7   

@ernilambar
Copy link
Copy Markdown
Member Author

Since we don't have different severity level in our checks currently I have not added any tests. After this PR is merged #518 (some strange issue has appeared), we can add tests with multiple severity levels.

@ernilambar ernilambar marked this pull request as ready for review July 18, 2024 07:35
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jul 18, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: ernilambar <rabmalin@git.wordpress.org>
Co-authored-by: davidperezgar <davidperez@git.wordpress.org>
Co-authored-by: swissspidy <swissspidy@git.wordpress.org>
Co-authored-by: barrykooij <barrykooij@git.wordpress.org>
Co-authored-by: joemcgill <joemcgill@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@ernilambar ernilambar force-pushed the 479-add-severity-level branch from aa03633 to eac393c Compare July 19, 2024 04:56
@ernilambar
Copy link
Copy Markdown
Member Author

PR has been updated with feature tests.

@davidperezgar davidperezgar requested a review from barrykooij July 21, 2024 09:59
@davidperezgar
Copy link
Copy Markdown
Member

For me it seems correct. Where do we put the severity in check? I'd update some documentation in the plugin about severity levels.

@ernilambar
Copy link
Copy Markdown
Member Author

ernilambar commented Jul 22, 2024

For sniffer rules, we can assign severity like this in phpcs-rulesets/plugin-review.xml:

<rule ref="WordPress.WP.PostsPerPage">
  <severity>9</severity>
</rule>

For other type of check like add_result_error_for_file() or add_result_warning_for_file(), there is addition of new parameter for severity

$this->add_result_error_for_file(
  $result,
  __( 'Prohibited text found.', 'pcp-addon' ),
  'prohibited_text_detected',
  $file,
  0,
  0,
  8
);

In this example, 8 is set as severity for the prohibited_text_detected check.

For checks which are extended from Abstract_PHP_CodeSniffer_Check, like following, I could not find the way to change the severity level:

protected function get_args() {
	return array(
		'extensions' => 'php',
		'standard'   => 'WordPress',
		'sniffs'     => 'WordPress.WP.I18n',
	);
}

@ernilambar
Copy link
Copy Markdown
Member Author

Currently several readme warnings are kept under same error code readme_parser_warnings. If we need to have separate severity level for separate error code then we need to keep unique error code for each warnings.
Ref: https://github.com/WordPress/plugin-check/blob/trunk/includes/Checker/Checks/Plugin_Repo/Plugin_Readme_Check.php#L538

@davidperezgar
Copy link
Copy Markdown
Member

I think they are all Warnings.

@davidperezgar
Copy link
Copy Markdown
Member

Thanks Nilambar, you have solved the conflicts and we have now new argument for severity.

@swissspidy
Copy link
Copy Markdown
Member

What about the suggestion at #479 (comment) to have separate severity levels for warnings and errors

@ernilambar
Copy link
Copy Markdown
Member Author

PR updated with feature tests and also with separate severity arguments.

@swissspidy swissspidy requested a review from joemcgill August 9, 2024 08:24
@davidperezgar
Copy link
Copy Markdown
Member

Why did you separate the severity?

I undestood that 0-5 is warning, and 6-10 is error. After that, we have different severity levels. I think is less complicated.

@swissspidy
Copy link
Copy Markdown
Member

This was suggested at #479 (comment)

let‘s discuss it there

seperate levels brings more flexibility

@ernilambar
Copy link
Copy Markdown
Member Author

Why did you separate the severity?

I undestood that 0-5 is warning, and 6-10 is error. After that, we have different severity levels. I think is less complicated.

This is not the approach this PR has implemented. Since our significant checks used PHPCS, it'd better we follow the similar approach it follows.

@davidperezgar
Copy link
Copy Markdown
Member

Ok

Copy link
Copy Markdown
Member

@swissspidy swissspidy left a comment

Choose a reason for hiding this comment

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

Looks good to me but would appreciate more reviews.

@davidperezgar
Copy link
Copy Markdown
Member

Ok, I ping to others.

@barrykooij
Copy link
Copy Markdown
Member

barrykooij commented Aug 14, 2024

Look great. I agree keeping separate levels for warnings and errors for that extra bit of flexibility. For plugin repo specific checks, we'll have an extra look afterward if all levels are the way we like, but that's non-blocking for this PR.

@davidperezgar
Copy link
Copy Markdown
Member

Perfect!

@swissspidy swissspidy added this to the 1.1.0 milestone Aug 14, 2024
@swissspidy swissspidy changed the title Add severity level in check Add support for severity levels for warnings and errors Aug 14, 2024
@swissspidy swissspidy merged commit ec0557e into trunk Aug 14, 2024
@swissspidy swissspidy deleted the 479-add-severity-level branch August 14, 2024 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Create a better solution for severity levels

4 participants