hynek / structlog

Simple, powerful, and fast logging for Python.
https://www.structlog.org/
Other
3.6k stars 225 forks source link

Type hint vs Example mismatch #660

Closed miketheman closed 1 month ago

miketheman commented 1 month ago

Hello! With structlog 24.4.0, I was combing through some mypy lint issues, and found that the example shown in the docs is basically what raises the issue, and wanted to understand which is intended to be correct.

Snippet taken from https://www.structlog.org/en/stable/api.html#structlog.processors.JSONRenderer specifically: https://github.com/hynek/structlog/blob/8a31eaaac810b4c3a271f903a430735f3445e29f/docs/api.rst?plain=1#L154-L155

$ mypy -c 'from structlog.processors import JSONRenderer; JSONRenderer(sort_keys=True)(None, None, {"a": 42, "b": [1, 2, 3]})'
<string>:1: error: Argument 2 to "__call__" of "JSONRenderer" has incompatible type "None"; expected "str"  [arg-type]

Tested with mypy 1.10.1 and 1.11.2 (latest) - both show the same problem.

Considering the second argument to __call__: https://github.com/hynek/structlog/blob/8a31eaaac810b4c3a271f903a430735f3445e29f/src/structlog/processors.py#L341

hynek commented 1 month ago

Hm, hard to say, the example just doesn't care, but maybe I should change it to ""?

The argument is the name of the logging method so I think it's better to keep it tight and let it be str – in what real-world scenario was that a problem? Since typing came many years after structlog's birth, not everything is super tight…

miketheman commented 1 month ago

Hm, hard to say, the example just doesn't care, but maybe I should change it to ""?

If that is a no-op change, then sure, the example should be updated. I guess not having an example of what that should be would help as well?

The argument is the name of the logging method ...

There's some magic that I'm not understanding on how that works, but that's okay, I don't need to! 😉 As long as it works...

in what real-world scenario was that a problem?

Observed here: https://github.com/pypi/warehouse/blob/10570d141ceb7377b374b425f80a4bf284df126c/warehouse/logging.py#L40

If replacing the effective name=None with name="" has no bearing on the overall behavior, then I'm happy to change it - but I wanted to have this conversation resolve before I did anything.

hynek commented 1 month ago

I think you're overestimating what that argument does or where it's coming from. :)

It's just a promise from structlog's bound loggers to pass it into the processor chain, because the bound loggers know what has been called.

If your processors don't use the argument, don't worry about it, but in your example the correct way to call RENDERED would be RENDERER(None, record.levelname, event_dict).

miketheman commented 1 month ago

If your processors don't use the argument, don't worry about it, but in your example the correct way to call RENDERED would be RENDERER(None, record.levelname, event_dict).

Thanks! I'll do that.

And since I saw that you've shipped the update to the examples, I think this is closed!