Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-93343: Expand warning filter examples#106618

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
daniel-shimon wants to merge4 commits intopython:main
base:main
Choose a base branch
Loading
fromdaniel-shimon:gh-93343-extra-warning-examples

Conversation

daniel-shimon
Copy link

@daniel-shimondaniel-shimon commentedJul 11, 2023•
edited by github-actionsbot
Loading

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 commentedJul 11, 2023•
edited by ghost
Loading

All commit authors signed the Contributor License Agreement.
CLA signed

Copy link
Member

@ezio-melottiezio-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 inpytest-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 thewarnings docs -- either a genericExamples 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 thewarnings tests inLib/test.

@@ -214,6 +214,18 @@ Some examples::
ignore,default:::mymodule # Only report warnings triggered by "mymodule"
error:::mymodule # Convert warnings to errors in "mymodule"

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? Ther here is not necessary, and its usage should be consistent between the two args (assuming they are both regexes).

erlend-aasland reacted with thumbs up emoji

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 ;)


filterwarnings("ignore", message=".*generic", module=r"yourmodule\.submodule")
# Ignore warnings in "yourmodule.submodule" which contain "generic"
filterwarnings("ignore", module="yourmodule.*")
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", module="yourmodule.*")
filterwarnings("ignore", module="yourmodule\.*")

I think this should be escaped too.

erlend-aasland reacted with thumbs up emoji
Copy link
Contributor

@erlend-aaslanderlend-aaslandJul 11, 2023•
edited
Loading

Choose a reason for hiding this comment

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

Also, instead of using comments, would it be clearer (formatting wise) to use multiple code blocks? (Goes for the change below as well)

Choose a reason for hiding this comment

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

No because here we actually want to catch all characters after 'yourmodule' and not only dot

Copy link
Contributor

Choose a reason for hiding this comment

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

    Add examples of warning filters and the difference between programatic and environmental filters.
@daniel-shimondaniel-shimonforce-pushed thegh-93343-extra-warning-examples branch from826cd01 toe43adf8CompareJuly 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 usegit pull --no-ff main instead. See alsohttps://devguide.python.org/getting-started/pull-request-lifecycle/

@erlend-aasland
Copy link
Contributor

erlend-aasland commentedJul 12, 2023•
edited
Loading

Oh, it seems like none of the example code inDoc/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 reacted with confused emoji

@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 and CharlieZhao95 reacted with heart emoji

@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 :)

daniel-shimon and blaisep reacted with thumbs up emoji

@hugovk
Copy link
Member

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!

erlend-aasland and mattwang44 reacted with thumbs up emoji

Copy link
Member

@hugovkhugovk 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

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 phraseI 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.

@hugovkhugovk added the pendingThe issue will be closed if no feedback is provided labelOct 24, 2024
@donBarbos
Copy link
Contributor

another half year has passed :)
I don't know how long it should take after addingpending 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!

donBarbos and hugovk reacted with thumbs up emoji

@daniel-shimon
Copy link
Author

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

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

@ezio-melottiezio-melottiezio-melotti left review comments

@hugovkhugovkhugovk requested changes

@erlend-aaslanderlend-aaslandAwaiting requested review from erlend-aasland

@CAM-GerlachCAM-GerlachAwaiting requested review from CAM-Gerlach

Assignees
No one assigned
Labels
awaiting changesdocsDocumentation in the Doc dirpendingThe issue will be closed if no feedback is provided
Projects
Status: Todo
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

7 participants
@daniel-shimon@erlend-aasland@CharlieZhao95@hugovk@donBarbos@ezio-melotti@bedevere-bot
[8]ページ先頭
©2009-2025 Movatter.jp