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

Type hint vs Example mismatch #660

Closed
miketheman opened this issue Oct 11, 2024 · 4 comments
Closed

Type hint vs Example mismatch #660

miketheman opened this issue Oct 11, 2024 · 4 comments

Comments

@miketheman
Copy link

miketheman commented Oct 11, 2024

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:

structlog/docs/api.rst

Lines 154 to 155 in 8a31eaa

>>> from structlog.processors import JSONRenderer
>>> JSONRenderer(sort_keys=True)(None, None, {"a": 42, "b": [1, 2, 3]})

$ 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__:

self, logger: WrappedLogger, name: str, event_dict: EventDict

  • Should the type be str | None or should the example be updated?
  • Is the name value used at all, or could that be turned into _name: Any or something to tell everyone we don't care about this variable?
@hynek
Copy link
Owner

hynek commented Oct 12, 2024

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…

hynek added a commit that referenced this issue Oct 12, 2024
To not violate our own type hints and intentions.

Ref #660
@miketheman
Copy link
Author

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
Copy link
Owner

hynek commented Oct 16, 2024

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
Copy link
Author

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!

miketheman added a commit to miketheman/warehouse that referenced this issue Oct 26, 2024
Refs: hynek/structlog#660 (comment)

Provide context links whenever possible.

Signed-off-by: Mike Fiedler <[email protected]>
miketheman added a commit to miketheman/warehouse that referenced this issue Oct 26, 2024
Refs: hynek/structlog#660 (comment)

Provide context links whenever possible.

Signed-off-by: Mike Fiedler <[email protected]>
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

2 participants