Skip to content

Don't assume that LogRecord.args is a tuple #12752

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 1 commit into from
Jul 13, 2024

Conversation

jfly
Copy link
Contributor

@jfly jfly commented Jun 6, 2024

In the context of pip, and especially when rich=True, I bet it is true that LogRecord.args is always a tuple.

However, in general, this is not true. Single argument dicts actually are treated specially by Python's logging
infrastructure
:

>>> def build_record(*args):
...   return LogRecord(name="name", level=0, pathname="pathname", lineno=42, msg="msg", args=args, exc_info=False).args
...
>>> build_record({"foo": 42}, {"bar": 43})
({'foo': 42}, {'bar': 43})          # <-- this is a tuple!
>>> build_record(42)
(42,)                               # <-- this is a tuple!
>>> build_record({"foo": 42})
{'foo': 42}                         # <-- this is not a tuple!

This fixes #12751

While I was in here, I also changed our logging story so we can actually capture the full stacktrace when logging is turned up. That would have been quite useful for me when debugging this issue, and would also be useful for folks debugging keyring backends. Hopefully this is uncontroversial. Happy to split this out into a separate PR if folks prefer.

In the context of pip, and especially when `rich=True`, I bet it *is*
true that `LogRecord.args` is always a tuple.

However, in general, this is not true. Single argument dicts actually
[are treated specially by Python's logging
infrastructure](https://github.com/python/cpython/blob/v3.11.9/Lib/logging/__init__.py#L301-L320):

    >>> def build_record(*args):
    ...   return LogRecord(name="name", level=0, pathname="pathname", lineno=42, msg="msg", args=args, exc_info=False).args
    ...
    >>> build_record({"foo": 42}, {"bar": 43})
    ({'foo': 42}, {'bar': 43})          # <-- this is a tuple!
    >>> build_record(42)
    (42,)                               # <-- this is a tuple!
    >>> build_record({"foo": 42})
    {'foo': 42}                         # <-- this is not a tuple!

This fixes pypa#12751

While I was in here, I also changed our logging story so we can actually
capture the full stacktrace when logging is turned up. That would have
been quite useful for me when debugging this issue, and would also be
useful for folks debugging keyring backends. Hopefully this is
uncontroversial. Happy to split this out into a separate PR if folks
prefer.
Copy link
Member

@ichard26 ichard26 left a comment

Choose a reason for hiding this comment

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

Thanks!

@ichard26 ichard26 added this to the 24.2 milestone Jul 11, 2024
@pradyunsg pradyunsg merged commit 58aacf0 into pypa:main Jul 13, 2024
29 checks passed
@pradyunsg
Copy link
Member

Thanks @jfly!

@jfly jfly deleted the issue-12751-incorrect-logging-assertion branch July 13, 2024 21:58
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

keyring import provider fails if pip is run with -vv or -vvv
4 participants