Skip to content

Update logging LogRecord msg attribute #9914

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

Merged
merged 3 commits into from
Mar 21, 2023
Merged

Conversation

aminalaee
Copy link
Contributor

Closes #9913

@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Member

AlexWaygood commented Mar 21, 2023

It looks like this would be a partial revert of #1776 by @rowillia.

Since there are no mypy-primer hits, it seems unlikely that too many people are passing non-str objects in this specific situation. And the inconsistency with the attribute annotation is indeed incorrect -- they should either both be str or both be object.

@AlexWaygood
Copy link
Member

AlexWaygood commented Mar 21, 2023

But I'm not sure it makes sense to only do a partial revert of #1776. msg annotations should probably be the same across all of logging/__init__.pyi. For example, logging.Logger.debug currently uses object as the annotation for the msg parameter:

def debug(
self,
msg: object,

But logging.Logger.debug calls logging.Logger._log, which passes msg to logging._logRecordFactory, and logging._logRecordFactory is an alias for logging.LogRecord. So if you passed a non-str object to logging.Logger.debug, you'd end up passing a non-str object to the logging.LogRecord constructor. Meaning that the annotation for the msg parameter for logging.Logger.debug (and logging.Logger.{info,warning,warn,error,exception,critical,log,_log}) should be kept in sync with the annotation for the msg parameter of logging.LogRecord.__init__.

@AlexWaygood
Copy link
Member

In the issue, you wrote:

According to the Python docs for the LogRecord in logging module the msg argument and property is a string:
https://docs.python.org/3/library/logging.html#logging.LogRecord

However, the CPython docs are pretty inconsistent here: later on down in the docs, it's specifically stated that it's okay to pass non-str objects in as messages:

In the preceding sections and examples, it has been assumed that the message passed when logging the event is a string. However, this is not the only possibility. You can pass an arbitrary object as a message, and its __str__() method will be called when the logging system needs to convert it to a string representation. In fact, if you want to, you can avoid computing a string representation altogether - for example, the SocketHandler emits an event by pickling it and sending it over the wire.

This seems to me like a bug in the CPython docs: they shouldn't say one thing up at the top, only to say a different thing later down.

@aminalaee
Copy link
Contributor Author

aminalaee commented Mar 21, 2023

I hadn't really checked the other places, you're right.
This is much deeper in the docs than the CPython and typeshed. I will create another PR on CPython docs.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Since the CPython docs specifically say that it's okay to pass in arbitrary non-str objects to the msg parameter, I think a better fix would be to keep object for the parameter annotation here, but change this attribute annotation so that it is also object:

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thank you! A docs PR over at CPython would also be great. If you ping me on the PR, I can help review it :)

@aminalaee aminalaee changed the title Update logging LogRecord msg argument Update logging LogRecord msg attribute Mar 21, 2023
@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

logging module LogRecord __init__ mismatch
2 participants