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

Add PEP 561 marker #129

Merged
merged 6 commits into from Feb 6, 2022
Merged

Add PEP 561 marker #129

merged 6 commits into from Feb 6, 2022

Conversation

bringhurst
Copy link
Contributor

@bringhurst bringhurst commented Jan 21, 2022

  • Add a PEP 561 marker
  • Add basic mypy configuration
  • Run mypy from tox
  • Minor code modifications to make mypy happy

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 #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).

- Add a PEP 561 marker
- Add basic mypy configuration
- Run mypy from tox
- Minor code modifications to make mypy happy
@jenstroeger
Copy link

jenstroeger commented Feb 3, 2022

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

madzak commented Feb 5, 2022

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

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

bringhurst commented Feb 5, 2022

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:

python/typed_ast#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
Copy link
Contributor Author

bringhurst commented Feb 5, 2022

A new PR (no need to mix it up with type annotations) for dropping old python versions is over at #131

@jenstroeger
Copy 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.

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

@madzak
Copy link
Owner

madzak commented Feb 6, 2022

Thanks for the assistance, all setup and passing now!

@madzak madzak merged commit 838939b into madzak:master Feb 6, 2022
@bringhurst bringhurst deleted the mypy branch February 6, 2022 17:46
@jenstroeger
Copy link

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 #133 — is that where the rest of the typing work happens?

@bringhurst
Copy link
Contributor Author

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 #133 — is that where the rest of the typing work happens?

I was using #118 to track the overall effort.

#133 is just targeting one specific part (a weird corner case) of the overall effort.

@palfrey
Copy link

palfrey commented Apr 24, 2022

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 #118 (comment) as well)

@bringhurst
Copy link
Contributor Author

bringhurst commented Apr 27, 2022

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). :)

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

Successfully merging this pull request may close these issues.

None yet

4 participants