Skip to content
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

prevent kwarg clashes #372

Closed
lijok opened this issue Nov 24, 2021 · 2 comments
Closed

prevent kwarg clashes #372

lijok opened this issue Nov 24, 2021 · 2 comments

Comments

@lijok
Copy link

lijok commented Nov 24, 2021

I keep on running into this problem:

TypeError: BoundLogger._proxy_to_logger() got multiple values for argument 'method_name'
TypeError: BoundLogger._proxy_to_logger() got multiple values for argument 'method'

and some others, when trying to:

log.error("connector method invocation failed", method=connector.formal_method_name, method_path=connector.method_path, method_name=connector.method_name)

I've heard from collegues of them running into the same issues with other keywords, like event iirc and some others

My understanding is that most keyword clashes can be prevented by changing internal method definitions to, say, prepend each kwarg with an underscore?

Let me know your thoughts, and if you think this is reasonable, I'll raise a pr to fix this

@hynek
Copy link
Owner

hynek commented Nov 24, 2021

You're right, with your analysis, however the problem is that those method definitions aren't private at all.

_proxy_to_logger() is an official API for anyone implementing their own loggers and in hindsight it's a gross oversight to give the method the underscore but not to the arguments!

Event is even a bit more complicated, because it's part of the logging calls and it's syntactically impossible to pass a kw-arg called event to it.


Generally speaking, I would be interested to untangle all of this. But I'm afraid the only backward-compatible way would be to introduce new APIs that replace _proxy_to_logger and leave that one be. In fact, I wanted to get rid of all the subclassing shenanigans anyways.

@hynek hynek closed this as completed Jul 6, 2022
@PerchunPak
Copy link

Can it at least fail silently?

Like

class Logger:
    def _log(self, *args, **kwargs):
        try:
             self._real_log(*args, **kwargs)
        except Exception:
             self.exception("Internal logging error.")

PerchunPak added a commit to PerchunPak/pinger-bot that referenced this issue Dec 10, 2022
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

No branches or pull requests

3 participants