search

madzak / python-json-logger

Json Formatter for the standard python logger
BSD 2-Clause "Simplified" License
1.7k stars 231 forks source link

Add PEP 561 marker #129

Closed bringhurst closed 2 years ago

bringhurst commented 2 years ago

This is intended to be a relatively minimal change just to enable basic type checking. Future changes should fill out the type annotations more and enable stricter mypy configuration (set all mypy options to 'true').

This change is a small step towards https://github.com/madzak/python-json-logger/issues/118

Note that the marker is intentionally not shipped via setup.py -- the annotations need more cleanup first (turn on a few key mypy config options, e.g. no-untyped-defs).

jenstroeger commented 2 years ago

@madzak, any chance you could move forward with this PR?

@bringhurst, thank you for getting this started. Curious though, why didn’t you annotate all methods, e.g. add_fields() etc? Perhaps the stdlib annotations of the logging package over at typeshed might help — here’s the link.

madzak commented 2 years ago

@jenstroeger I had to merge in a previous commit from an earlier PR to get the build checks to run properly. Seems pypy3 and python 3.5 are failing. Can you look into the failure? If not i'll have some time later next week to dig in more.

jenstroeger commented 2 years ago

@madzak to be honest, before I start digging into the Python 3.5 syntax error which fails the tests it would make sense to consider retiring Python 3.5 support altogether (official link):

DEPRECATION: Python 3.5 reached the end of its life on September 13th, 2020. Please upgrade your Python as Python 3.5 is no longer maintained. pip 21.0 will drop support for Python 3.5 in January 2021. pip 21.0 will remove support for this functionality.

As for the pypy3.6 (3.6.12) failure, it fails to compile the typed-ast package because gcc misses the codecs.h include. I’ve not dug deeper into this yet.

bringhurst commented 2 years ago

Perhaps the stdlib annotations of the logging package over at typeshed might help — here’s the link.

I wasn't sure if there was interest in getting types added, so I didn't bother going through everything in detail. :) Since it looks like there's definitely interest, I'll run through it later this week and try to fill in all of the missing annotations.

As for the pypy3.6 (3.6.12) failure, it fails to compile the typed-ast package because gcc misses the codecs.h include. I’ve not dug deeper into this yet.

It appears that older versions of pypy (pre 3.8) don't seem to be very compatible with mypy. Some details can be found at:

https://mypy.readthedocs.io/en/stable/faq.html#does-it-run-on-pypy

and also:

https://github.com/python/typed_ast/issues/179

In addition to dropping python 3.5 support, maybe it could also be switched from pypy3 (3.6) to pypy38 instead? II haven't checked to see if this works, but it appears that it would resolve the current error.

bringhurst commented 2 years ago

A new PR (no need to mix it up with type annotations) for dropping old python versions is over at https://github.com/madzak/python-json-logger/pull/131

jenstroeger commented 2 years ago

I wasn't sure if there was interest in getting types added, so I didn't bother going through everything in detail. :) Since it looks like there's definitely interest, I'll run through it later this week and try to fill in all of the missing annotations.

@bringhurst — definitely interest, and I’m happy to lend a hand to get this done 🤓

madzak commented 2 years ago

Thanks for the assistance, all setup and passing now!

jenstroeger commented 2 years ago

Uhm… this is not a complete annotation, is it? There are still heaps of functions without type annotations, but I see that @bringhurst opened PR https://github.com/madzak/python-json-logger/pull/133 — is that where the rest of the typing work happens?

bringhurst commented 2 years ago

Uhm… this is not a complete annotation, is it? There are still heaps of functions without type annotations, but I see that @bringhurst opened PR https://github.com/madzak/python-json-logger/pull/133 — is that where the rest of the typing work happens?

I was using https://github.com/madzak/python-json-logger/issues/118 to track the overall effort.

https://github.com/madzak/python-json-logger/pull/133 is just targeting one specific part (a weird corner case) of the overall effort.

palfrey commented 2 years ago

AFAIK, this hasn't worked. Installing python-json-logger from current git master still gets Skipping analyzing "pythonjsonlogger": module is installed, but missing library stubs or py.typed marker. https://peps.python.org/pep-0561/#packaging-type-information indicates the missing feature is a package_data section in the setup.cfg (which I'm seeing in https://github.com/madzak/python-json-logger/issues/118#issue-878705313 as well)

bringhurst commented 2 years ago

AFAIK, this hasn't worked

Correct -- that error is the expected result after this PR.

133 is the next step in getting type hints exported (a small step in completing #118).

Note the comment above:

Note that the marker is intentionally not shipped via setup.py -- the annotations need more cleanup first (turn on a few key mypy config options, e.g. no-untyped-defs).

My thought behind this is that there's not much point in exporting inconsistent type hints. We should fix them before exporting (via package_data). :)