Is it right to deprecate skip_warningiserror (v8.1.x)?

[jdknight]

Recently been testing an extension with Sphinx's development branch and have just stumbled upon the recent changes to the handling of warnings. While the unit testing for this extension relies on exceptions for various checks, I have no problem with updating the testing logic to utilize the recently added exception_on_warning option.

When trying to verify all unit tests in the extension, additional changes were needed to handle that now deprecated skip_warningiserror definition. In the test environment, the default case would now enable both warningiserror and exception_on_warning. Several unit tests rely on skip_warningiserror -- which can be somewhat worked around by not enforcing this.

However, there is some logic in the non-testing side of this extension that utilizes skip_warningiserror. For example, there currently is a use case where when a HTTP rate-limit event is detected when publishing documentation, a warning is generated until a retry-event starts. If a user which observed a rate-limit event when processing documentation with --fail-on-warnings set, the resulting run will be flagged as a failure as of the prospect v8.1.x release.

I feel like there is still value in having/supporting a skip_warningiserror call for use cases like this. That being said, I can see the case where the Sphinx project does not want to deal with the idea of "critical warnings" over "relaxed warnings". For the extension, we can update the logging to use info instead, but it is not the preferred level for events such as this.

[AA-Turner]

does not want to deal with the idea of "critical warnings" over "relaxed warnings"

This is one of the reasons I'm not a massive fan of -W, you loose discretion about what category a log message is...

I removed skip_warningiserror to reduce implementation complexity. We could restore it but I would prefer not to.

Suggestions:

1. Use INFO (HTTP 429 is sort-of fine anyway, its's not a failure)
2. We restore skip_warningiserror
3. We add a per-log-call opt-out of raising exceptions. (e.g. logger.warning(..., no_exception_on_warning=True) or a better name)

A

[jdknight]

I removed skip_warningiserror to reduce implementation complexity

I'm down for reducing implementation complexity.

It is fine by me for having it not restored. I mainly brought this up as an observed issue from the original change. If changing the subset of messages previously wrapped in skip_warningiserror to non-warning messages makes everyone's life easier, I'll go ahead and make the update extension.

HTTP 429 is sort-of fine anyway, its's not a failure

For sure not a failure... that's why it was logged as a warning instead of an error. :)

Humor aside, the are other message scenarios in the extension for other codes (such as 429 and 503, which may not end in a failure case). But all use cases in the extension in question can be re-worked. I'm not sure if there are other extensions out there relying on the same capability (for all I know, maybe it's none).

[AA-Turner]

I'll leave this open for others to chime in, note I am looking to release 8.1 next week so we should resolve this either way before that.

From a GitHub search, it seems there are only two non-test current uses of skip_warningiserror:

• oslo.policy (https://github.com/openstack/oslo.policy/blob/206ae19f48dfcd5c2fb34439de02694b1a78594e/oslo_policy/sphinxext.py#L160-L161)
• sphinxcontrib-confluencebuilder (https://github.com/sphinx-contrib/confluencebuilder/blob/v2.7.1/sphinxcontrib/confluencebuilder/rest.py#L200-L202, etc)

A

[jdknight]

The sphinxcontrib-confluencebuilder extension has been updated to no longer depend on skip_warningiserror. A release will be made next week, most likely around when a Sphinx 8.1 release will be made.