CLI: Treat errors below severity threshold as warnings

[davidperezgar]

Fixes #691

[github-actions]

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: davidperezgar <[email protected]>
Co-authored-by: swissspidy <[email protected]>
Co-authored-by: ernilambar <[email protected]>
Co-authored-by: felixarntz <[email protected]>
Co-authored-by: mukeshpanchal27 <[email protected]>
Co-authored-by: frantorres <[email protected]>

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

[ernilambar]

Current approach of severity is aligned with the PHPCS which could be considered as an industry standard. I would not want to break that approach and expectation.
IMO, we could add some flags to get the required results with errors converted to warnings.
Anyway lets wait for other opinions also.

[davidperezgar]

Ok @ernilambar I agree with you as you tell that is a industry standard. We all want the same results. Which name of the flag do you recommend? something like --show-error-warning-low-severity ?

[felixarntz]

@davidperezgar I'm not sure I fully understand the problem that this is trying to solve. Is the problem that some $results are removed because of the filtering? That seems reasonable to fix.

However, even with this PR implementation, some results would still be missing, which would be warnings with a lower severity.

I'm not sure that we should "migrate" low-severity errors to warnings. That seems like an odd choice. Would be great to get other people's feedback on this.

[davidperezgar]

Thanks @felixarntz . The thing is that we have errors that could have false positives. That's why we use the severity 7 in plugin submission, so the errors with severity less than 7, won't be a blocker. In our team we try to developers to learn about the errors, and it would be very helpful for the team, show that errors, and they could be a warning. I think is the best scenario.

Otherwise, they would find that issues does not a appear and they could not manage to solve them.

I agree with @ernilambar to use a new flag to do that.

[davidperezgar]

I've commited my proposal. To have less severity errors as warnings with the flag: --include-error-severity. That will include less severity errors as warnings.

[swissspidy]

A Behat test covering this change would be beneficial.

[davidperezgar]

Ok, I'm going to make it then.

[felixarntz]

I like the approach using a different type.

How about this?

• Use custom type ERRORS_LOW_SEVERITY for low severity errors.
• Do the same for warnings, i.e. add a flag --include-low-severity-warnings, and if that is set, include low severity warnings under WARNINGS_LOW_SEVERITY.

WDYT?

[davidperezgar]

Hello,
I see that is a good approach and all will be happy with that solution.

[davidperezgar]

Done! and ready to review!

[davidperezgar]

It's strange as it works BEHAT tests in my local installation, but is failing.

[ernilambar]

It's strange as it works BEHAT tests in my local installation, but is failing.

composer run behat -- tests/behat/features/plugin-check-severity.feature is not passing locally in my local setup.

[davidperezgar]

Ok Thanks Nilambar

[davidperezgar]

Finished!

[ernilambar]

Looks good.

[ernilambar]

CC @felixarntz for re-review. 😊

[felixarntz]

Just one note on WPCS, otherwise LGTM!

[davidperezgar]

Thanks!

[dd32]

Any reason these output types are pluralised?

ie. The types are now:

• ERROR
• ERRORS_LOW_SEVERITY
• WARNING
• WARNING_LOW_SEVERITY

[ernilambar]

Followup PR - #818