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 API type definitions through stub files #207

Merged
merged 35 commits into from
Sep 5, 2021

Conversation

AA-Turner
Copy link
Contributor

@daviddrysdale

xref #200
closes #202
closes #203

This PR has the stubs, automatic checking of the stubs with mypy.stubgen, and changes to setup.py to support packaging type stubs (I think I made the changes to the right setup.py, I was mildly confused as there's another one at the top level)

A

@AA-Turner AA-Turner changed the title Stubs Add API type definitions through stub files Sep 1, 2021
Copy link
Owner

@daviddrysdale daviddrysdale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for preparing this!

Just a couple of initial questions, without looking at the .pyi file details as yet.

I think I made the changes to the right setup.py, I was mildly confused as there's another one at the top level

IIRC that got added (#7) to help out folk who pointed installers at the top-level GitHub URL, which would then fail because the code is in a subdirectory. (I've added #209 as a reminder to sort this out)

python/run_stubtest.py Outdated Show resolved Hide resolved
python/setup.py Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
daviddrysdale added a commit that referenced this pull request Sep 2, 2021
Contents of generated files taken from #207
daviddrysdale added a commit that referenced this pull request Sep 4, 2021
Contents of generated files taken from #207
daviddrysdale added a commit that referenced this pull request Sep 4, 2021
Contents of generated files taken from #207
Copy link
Owner

@daviddrysdale daviddrysdale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few questions from an initial pass (just looking over the smaller files).

python/phonenumbers/geocoder.pyi Outdated Show resolved Hide resolved
python/phonenumbers/geocoder.pyi Show resolved Hide resolved
python/phonenumbers/util.pyi Show resolved Hide resolved
python/phonenumbers/timezone.pyi Show resolved Hide resolved
python/phonenumbers/carrier.pyi Show resolved Hide resolved
python/phonenumbers/util.pyi Show resolved Hide resolved
python/phonenumbers/util.pyi Show resolved Hide resolved
AA-Turner pushed a commit to AA-Turner/python-phonenumbers that referenced this pull request Sep 4, 2021
Contents of generated files taken from daviddrysdale#207
Copy link
Owner

@daviddrysdale daviddrysdale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates; a couple more questions as I work through the PR.

@@ -0,0 +1,82 @@
from .asyoutypeformatter import AsYouTypeFormatter as AsYouTypeFormatter
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the X as X parts needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is required, although looking at the docs it seems it might also be possible to put in __all__ -- though in other stub files I've seen it like this and thought it was the canonical approach

https://mypy.readthedocs.io/en/stable/config_file.html#confval-implicit_reexport (note the last line that implicit re-export is disabled for stubs -- the docs could be written more clearly here on the inverse of explicit re-exports)

from .phonemetadata import NumberFormat as NumberFormat
from .phonemetadata import PhoneMetadata as PhoneMetadata
from .phonemetadata import PhoneNumberDesc as PhoneNumberDesc
from .phonemetadata import REGION_CODE_FOR_NON_GEO_ENTITY as REGION_CODE_FOR_NON_GEO_ENTITY
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe make the layout of the imports match __init__.py more closely?

Or is this (and the X as X parts) something tool-derived that's easier to leave as-is?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The from X import Y as Y was autogenerated, yes -- mypy.stubgen was able to infer a lot of stuff, but I had to manually correct a lot of types, and all private variables/class variables/functions I had to hand write, as stubgen didn't pick them up.

python/phonenumbers/phonenumber.pyi Outdated Show resolved Hide resolved
python/phonenumbers/util.pyi Show resolved Hide resolved
python/phonenumbers/util.pyi Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
Copy link
Owner

@daviddrysdale daviddrysdale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Phew, I've finished looking over the .pyi files! (I still want to check over the packaging changes btw)

python/phonenumbers/shortnumberinfo.pyi Show resolved Hide resolved
python/phonenumbers/phonenumberutil.pyi Show resolved Hide resolved
python/phonenumbers/phonenumberutil.pyi Show resolved Hide resolved
python/phonenumbers/phonenumberutil.pyi Show resolved Hide resolved
daviddrysdale added a commit that referenced this pull request Sep 5, 2021
Contents of generated files taken from #207
@daviddrysdale daviddrysdale merged commit 91abecb into daviddrysdale:dev Sep 5, 2021
@septatrix
Copy link

Given that there is a debian directory in this repo are you responsible for publishing new versions?

@daviddrysdale
Copy link
Owner

Given that there is a debian directory in this repo are you responsible for publishing new versions?

Nope. I included the debian/ at the request (#39) of someone else, but there's not been a significant update here since ~2015. Maybe check the Debian package info?

DMRobertson pushed a commit to matrix-org/synapse that referenced this pull request May 3, 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

Successfully merging this pull request may close these issues.

3 participants