Skip to content

Conversation

@daniel-shimon
Copy link

@daniel-shimon daniel-shimon commented Jul 11, 2023

Add examples of warning filters and the difference between programatic and environmental filters.


📚 Documentation preview 📚: https://cpython-previews--106618.org.readthedocs.build/

@ghost
Copy link

ghost commented Jul 11, 2023

All commit authors signed the Contributor License Agreement.
CLA signed

Copy link
Member

@ezio-melotti ezio-melotti left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!
This is a step in the right direction, but there are more subtleties in pytest-dev/pytest#8759 (comment) that are not described here.

In particular:

  • Where/when can I uses regexes vs plain text? This applies to both different types of filters (filterwarnings()/-W/PYTHONWARNINGS) and different fields within a filter (message/package/module).
  • How to match part of a message with the different types of filters (beginning of the message vs substring).
  • How to filter messages coming from specific packages/modules/submodules with the different types of filters.

I think this would be better covered in a new section of the warnings docs -- either a generic Examples section that includes these alongside with comments and explanations, or a more specific section that focuses on the differences between the different warnings types. A mini-howto might also be an option, but we can start with a single section and expand it later.

In addition, since you would have to test different filters to ensure that the behavior you are documenting is correct, it might be worth adding some of these examples to the warnings tests in Lib/test.


Note that :func:`filterwarnings` filters have subtle differences from :option:`-W` and :envvar:`PYTHONWARNINGS`::

filterwarnings("ignore", message=".*generic", module=r"yourmodule\.submodule")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
filterwarnings("ignore", message=".*generic", module=r"yourmodule\.submodule")
filterwarnings("ignore", message="\.*generic", module="yourmodule\.submodule")

Shouldn't this be escaped as well? The r here is not necessary, and its usage should be consistent between the two args (assuming they are both regexes).

Copy link
Author

Choose a reason for hiding this comment

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

No since the first arg wants to capture strings that contain 'generic' so that the '.' catches everything, while the first arg want to capture 'yourmodule.submodule' specifically, meaning that the '.' actually captures dot and should be escaped

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, it sounds to me you should put that explanation into the docs, @daniel-shimon; if it is unclear for a reviewer, it is not going to be clear for the average docs reader ;)

@daniel-shimon daniel-shimon force-pushed the gh-93343-extra-warning-examples branch from 5c2f95e to 826cd01 Compare July 12, 2023 13:52
    Add examples of warning filters and the difference between programatic and environmental filters.
@daniel-shimon daniel-shimon force-pushed the gh-93343-extra-warning-examples branch from 826cd01 to e43adf8 Compare July 12, 2023 14:08
@daniel-shimon
Copy link
Author

Hi @ezio-melotti, thanks for the fast review!
I moved the examples to a separate section and elaborated a bit on the explanations.

Where/when can I uses regexes vs plain text? This applies to both different types of filters (filterwarnings()/-W/PYTHONWARNINGS) and different fields within a filter (message/package/module).
How to match part of a message with the different types of filters (beginning of the message vs substring).

I've linked to the warning filter definitions since I think they explain this concisely and don't think we need to add another section repeating the same info. What do you think?

@erlend-aasland
Copy link
Contributor

FYI, @daniel-shimon: please don't force push; it does not play well with the GitHub UI, and especially not with review remarks and CI. You can use git pull --no-ff main instead. See also https://devguide.python.org/getting-started/pull-request-lifecycle/

@erlend-aasland
Copy link
Contributor

erlend-aasland commented Jul 12, 2023

Oh, it seems like none of the example code in Doc/library/warnings.rst is checked by doctest :( That is unfortunate! I don't remember the doctest specifics, but if it is possible, please make is so that at least the added examples are run through doctest. If not, adding doctests to this .rst file is out of scope for this PR, but it should be done.

@daniel-shimon
Copy link
Author

@erlend-aasland so actually this was because I amended the commit, cause I thought it was weird to have "pr fixes" in the main Python repo after accepting the pr.
So should I add new commits and squash before merge? What is the practice here?

@CharlieZhao95

This comment was marked as resolved.

@erlend-aasland

This comment was marked as resolved.

@daniel-shimon
Copy link
Author

Ok I see and understand that now, thanks guys 🙌

@erlend-aasland
Copy link
Contributor

Ok I see and understand that now, thanks guys 🙌

np, and thanks for improving the docs! I recommend spending some hours with the devguide; there's a lot of useful information there :) Also, if you find sections in the devguide that are hard to grasp, or just worded badly, don't hesitate to open an issue over at the devguide repo; fixing devguide bugs is one of the most important things we can do :)

@hugovk
Copy link
Member

hugovk commented Jul 14, 2024

Hello from +1 year! Where are we up to on this PR? It looks like there's still some suggestions for @daniel-shimon to address, would you like update the PR? Thanks!

Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

Ping :)

@bedevere-app
Copy link

bedevere-app bot commented Oct 24, 2024

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@hugovk hugovk added the pending The issue will be closed if no feedback is provided label Oct 24, 2024
@donbarbos
Copy link
Contributor

another half year has passed :)
I don't know how long it should take after adding pending label, but I suggest waiting the last 2 weeks (another attempt @daniel-shimon)

@daniel-shimon
Copy link
Author

Hi!
Wow somehow I've missed the emails about this PR and was sure it was long forgotten 😅
I'll address the comments in the upcoming days, thanks for the ping!

@daniel-shimon
Copy link
Author

Hi @erlend-aasland , I added clarifications regarding escaping and regexes.
Could you take a look?
Thanks!

@picnixz picnixz removed the pending The issue will be closed if no feedback is provided label Nov 15, 2025
Comment on lines 250 to 251
# in order to match a literal dot character.
filterwarnings("ignore", message="generic", module=r"yourmodule\.submodule")
Copy link
Member

Choose a reason for hiding this comment

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

I think those examples should be separated by a new line for clarity purposes.


def test_string_literals(self):
# Ensure message/module are treated as string literals
rc, stdout, stderr = assert_python_ok("-c",
Copy link
Member

Choose a reason for hiding this comment

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

If you use rc, check its value as well. Otherwise use _, stdout, stderr = ...

Copy link
Author

@daniel-shimon daniel-shimon left a comment

Choose a reason for hiding this comment

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

Fix CR Comments

@daniel-shimon
Copy link
Author

Hi @picnixz, thanks for the review! I've fixed your comments 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting changes docs Documentation in the Doc dir

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

8 participants