Skip to content

Show warnings with their "type" and "subtype"? #8845

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

Closed
mgeier opened this issue Feb 7, 2021 · 7 comments · Fixed by #12131
Closed

Show warnings with their "type" and "subtype"? #8845

mgeier opened this issue Feb 7, 2021 · 7 comments · Fixed by #12131
Labels
api:cmdline type:enhancement enhance or introduce a new feature

Comments

@mgeier
Copy link
Contributor

mgeier commented Feb 7, 2021

Similar to #8813, just for warnings instead of exceptions:

Currently, warning messages are shown like this:

WARNING: Some warning message

If a warning can be disabled, I think it would be helpful to display the warning type and subtype, maybe like:

WARNING (some_type.some_subtype): Some warning message
@mgeier mgeier added the type:enhancement enhance or introduce a new feature label Feb 7, 2021
@tk0miya
Copy link
Member

tk0miya commented Feb 7, 2021

-0: IMO, it's better to resolve the root reason for the warning instead of suppressing. I feel it's noisy...

@chrisjsewell
Copy link
Member

+1

For myst-parser and myst-nb I have had numerous people ask to silence a specific warning.
Although I encourage them to fix the warning, it is true that some warnings are more important than others. Also, as already mentioned, displaying the warning type helps to identify its origin, and thus how to fix it.

Displaying warning types has plenty of precedence in other packages, such as:

In myst-parser I now append the warning type to the end of the message: https://myst-parser.readthedocs.io/en/latest/using/howto.html#suppress-warnings

WARNING: Non-consecutive header level increase; 1 to 3 [myst.header]

I feel it's noisy...

You could simply add a configuration option to control whether they are displayed, e.g. show_warning_type

Also to note, in https://www.sphinx-doc.org/en/master/usage/configuration.html#confval-suppress_warnings it says "Now, this option should be considered experimental.".
I assume this is no longer the case, since this option has been around for a long time now?

@chrisjsewell
Copy link
Member

chrisjsewell commented Feb 15, 2021

I would consider creating a PR, but obviously would want to know first it was not likely to be rejected 😬

@mgeier
Copy link
Contributor Author

mgeier commented Feb 15, 2021

@tk0miya

IMO, it's better to resolve the root reason for the warning instead of suppressing.

I agree, most of the times it's better to fix the root reason.

But sometimes it's not, and then it would be nice to have the additional information in order to silence the right warnings.
If not, people might just silence all warnings, which might be worse!

So I would add to your argument:

  • most of the time, it's better to resolve the root reason for the warning instead of suppressing
  • if not, it's better to suppress only certain warnings explicitly instead of suppressing all of them

I feel it's noisy...

This would certainly add a little bit of noise, but I think it would be worth it.

For me, the main usage is in different CI tasks. I might run Sphinx in multiple CI tasks, but for certain builders (e.g. linkcheck) I might want to disable some warnings, which I want to keep enabled for the main html builder task.

Does this make sense?

@chrisjsewell

You could simply add a configuration option to control whether they are displayed, e.g. show_warning_type

This is certainly a possibility, but I feel that this would never actually be used (assuming it has the correct default value).

People who care about warning messages will keep them to a minimum (ideally 0) anyway. And people who don't care, won't care about the exact display of the warnings anyway.

@arwedus
Copy link

arwedus commented Nov 30, 2021

-0: IMO, it's better to resolve the root reason for the warning instead of suppressing. I feel it's noisy...

every C++ compiler gives an exact warning number, by which you can also disable the given warning type - if done right, the "noisiness" can be kept low and the output made really universal.

@felix-hilden
Copy link

@tk0miya I agree that it is better to fix the underlying issue. But to present another situation where it could be useful: I'm developing an extension that parses code blocks and links the code with corresponding autodoc entries. I intend to issue warnings when those entries are not found, but the code example might still be valid. So a user might want to ignore the warning because there's nothing they can do until the entry is added (perhaps in a 3rd party library). They can look at the extension documentation, but it would be much faster to see the exact warning type in the message.

It is a bit noisy though 😄 and I think more often it's a valid warning that can be fixed. So if not default behavior, could this at least be available via configuration?

@mara004
Copy link

mara004 commented Mar 1, 2022

I agree this would be useful, especially for working with myst. There are some references in my markdown files (in pypdfium2) that naturally can't be translated to Sphinx/rst. I'd like to suppress this noise to prevent me from overlooking real warnings.

@AA-Turner AA-Turner added this to the some future version milestone Sep 29, 2022
danwos pushed a commit to useblocks/sphinx-needs that referenced this issue Aug 21, 2023
This PR is intended to standardise and improve the sphinx warnings
emitted by sphinx-needs:

1. To make it clear when a warning originates from sphinx-needs
2. Where possible, to add the location of the warning origin (in the
source documentation)

i.e. all warning logs should now look like:

```
/path/to/file.rst:<line number>: WARNING: <message> [needs]
```

---

This is similar to my work in
https://myst-parser.readthedocs.io/en/latest/configuration.html#build-warnings,
it prefixes all warnings with `[needs]`, and adds the `needs` type to
the warning, meaning it can be specifically suppressed (see
https://www.sphinx-doc.org/en/master/usage/configuration.html#confval-suppress_warnings).
(warning subtypes could also be added in follow up PRs)

I have also added the `location` argument to some warnings (where the
location was obvious), although there are probably some more that could
also have a location (but this can be done in later PRs),

Note, IMO it would be ideal if the type was automatically shown by
sphinx, but this is not currently the case:
sphinx-doc/sphinx#8845

Note also, I intend to make an upstream PR to
https://github.com/sphinx-contrib/plantuml, to also improve their
warnings
@chrisjsewell chrisjsewell linked a pull request Mar 19, 2024 that will close this issue
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api:cmdline type:enhancement enhance or introduce a new feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants