-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
MAINT: don't format logs in log call. #25073
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
Conversation
In particular f-string: There are many reason not to use f-string in logs, - F-strings are eager, so might be slow. So this may affect performance (we can filter before formatting). - prevent structured logging or handler to highlight. - Security (untrusted input can lead to DOS on formatting, https://discuss.python.org/t/safer-logging-methods-for-f-strings-and-new-style-formatting/13802) But also % formatting in a couple of places.
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
Can a note about this be put somewhere on the contributing code page? I worry that this is the type of thing that will get changed in either direction 16 times. |
@meeseeksdev backport to 3.7.0 |
Something went wrong ... Please have a look at my logs. It seems that the branch you are trying to backport to does not exist. |
@meeseeksdev backport to 3.7.x |
Something went wrong ... Please have a look at my logs. It seems that the branch you are trying to backport to does not exist. |
@meeseeksdev backport to v3.7.x |
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
And apply the correct labels and milestones. Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon! Remember to remove the If these instructions are inaccurate, feel free to suggest an improvement. |
@Carreau is this important enough to manually backport? |
I didn't go back to find these lines specifically, but most of these are likely made into f strings via #24980 which was not itself backported to 3.7.x |
Ah.... It did all seem too obscure to have triggered conflicts. So I think this can safely not be backported. |
This may conflict with the f-string PR, but that PR only changed from |
Given it was non ideal before (and probably has been for a while) I do not think backporting is a high-priority. |
I agree it's not critical to backport. It can be put on the style guide, there is also a pylint item that detect some of those usage. Mostly it's giving the right example, so it has more chances of not being copy/pasted somewhere else. |
In particular f-string:
There are many reason not to use f-string in logs,
But also % formatting in a couple of places.
PR Checklist
Documentation and Tests
pytest
passes)Release Notes
.. versionadded::
directive in the docstring and documented indoc/users/next_whats_new/
.. versionchanged::
directive in the docstring and documented indoc/api/next_api_changes/
next_whats_new/README.rst
ornext_api_changes/README.rst