Skip to content

bpo-46557: Log captured warnings without format string (GH-30975)#30975

Merged
vsajip merged 6 commits into
python:mainfrom
mnito:fix-bpo-46557
Mar 15, 2022
Merged

bpo-46557: Log captured warnings without format string (GH-30975)#30975
vsajip merged 6 commits into
python:mainfrom
mnito:fix-bpo-46557

Conversation

@mnito

@mnito mnito commented Jan 28, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

For systems that aggregate logs like Sentry, the use of a format string causes all warnings to be grouped under the same issue.

https://bugs.python.org/issue46557

@mnito
mnito requested a review from vsajip as a code owner January 28, 2022 00:49
@the-knights-who-say-ni

the-knights-who-say-ni commented Jan 28, 2022

Copy link
Copy Markdown

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@mnito

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

mnito and others added 3 commits January 27, 2022 21:15
@mnito
bpo-46557: Log captured warnings without format string
1ae3478 
For systems that aggregate logs like Sentry, the use of a format
string causes all warnings to be grouped under the same issue.
@blurb-it @mnito
📜🤖 Added by blurb_it.
0f82fff
@mnito
Add period after blurb sentence
f777c46
ericvsmith
ericvsmith reviewed Jan 28, 2022
View reviewed changes
Comment thread Lib/logging/__init__.py
ericvsmith
ericvsmith reviewed Jan 28, 2022
View reviewed changes

@ericvsmith ericvsmith left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'll let @vsajip decide if this is a desirable feature. It doesn't seem unreasonable to me. But see my note about adding a comment in the code so that someone doesn't undo this change in the future.

@vsajip

vsajip commented Jan 28, 2022

Copy link
Copy Markdown
Member

I agree with Eric that it doesn't seem unreasonable, and although semantically there is a difference, in practice the scope of the change seems narrow. But I've marked the issue as an enhancement rather than behaviour and thus the changes to be limited to 3.11. (There's nothing wrong with the current behaviour, and Sentry could always treat "%s" as a special case at their end.)

@mnito

mnito commented Jan 28, 2022

Copy link
Copy Markdown
Contributor Author

I agree with Eric that it doesn't seem unreasonable, and although semantically there is a difference, in practice the scope of the change seems narrow. But I've marked the issue as an enhancement rather than behaviour and thus the changes to be limited to 3.11. (There's nothing wrong with the current behaviour, and Sentry could always treat "%s" as a special case at their end.)

Oh yeah absolutely, sorry that's just me being unfamiliar with filing Python issues. My intentions were not to have the change backported to earlier Python versions. Thanks!

@mnito
mnito requested a review from ericvsmith January 28, 2022 23:11
vsajip
vsajip requested changes Jan 29, 2022
View reviewed changes
Comment thread Lib/logging/__init__.py
@bedevere-bot

bedevere-bot commented Jan 29, 2022

Copy link
Copy Markdown

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.

@mnito

mnito commented Jan 29, 2022

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again

@bedevere-bot

bedevere-bot commented Jan 29, 2022

Copy link
Copy Markdown

Thanks for making the requested changes!

@vsajip: please review the changes made to this pull request.

@bedevere-bot
bedevere-bot requested a review from vsajip January 29, 2022 13:04
@vsajip vsajip changed the title bpo-46557: Log captured warnings without format string bpo-46557: Log captured warnings without format string (GH-30975) Mar 15, 2022
@vsajip
vsajip merged commit d8066b4 into python:main Mar 15, 2022
@mnito mnito mannequin mentioned this pull request Apr 10, 2022
Closed
@alcarney alcarney mentioned this pull request Nov 18, 2022
Closed
@FlorentClarret FlorentClarret mentioned this pull request Oct 17, 2023
Merged
3 tasks
@szokeasaurusrex szokeasaurusrex mentioned this pull request Apr 15, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vsajip vsajip vsajip approved these changes
@ericvsmith ericvsmith Awaiting requested review from ericvsmith
+1 more reviewer
@raghavthind2005 raghavthind2005 raghavthind2005 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@mnito @the-knights-who-say-ni @vsajip @bedevere-bot @ericvsmith @raghavthind2005