-
Notifications
You must be signed in to change notification settings - Fork 4k
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
THRIFT-5139/THRIFT-4181: Python3 type hints #2929
base: master
Are you sure you want to change the base?
Conversation
c14eb9f
to
f30be35
Compare
500f732
to
05d742d
Compare
It would be great to have this, it would almost enable transpilation via
|
test/py/Makefile.am
Outdated
gen-py-type_hints/%/__init__.py: ../%.thrift $(THRIFT) | ||
test -d gen-py-type_hints || $(MKDIR_P) gen-py-type_hints | ||
test ../v0.16/$(notdir $<) \ | ||
&& $(THRIFT) --gen py:type_hints -out gen-py-type_hints ../v0.16/$(notdir $<) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 LGTM just one question: UUIDs are handled (at least from what i see above) but you still want to test against the old uuid-free v0.16 version of IDL files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arkuhn Would like to merge this but ...
args not optional type hint UUID generator support Remove runtime type check
c6f54bc
to
3d1a741
Compare
- Update flag description to be more precise - No implict enum generation (gen enum flag required) - Use latest thrift test IDL for uuid coverage - rebase on latest main
3d1a741
to
b8d0da3
Compare
string t_py_generator::member_hint(t_type* type, t_field::e_req req) { | ||
if (gen_type_hints_) { | ||
if (req != t_field::T_REQUIRED) { | ||
return ": typing.Optional[" + type_to_py_type(type) + "]"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i wonder if we can just generate | None
if python version allows it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this would work, but only as of 3.10 (which is EOL in a couple months.) Using typing.*
and Union
syntax allows us to target all of 3.5 to 3.12+
Add type hinting and native enums (IntEnum) for python generated code
RunClientServer.py
andgenerate.cmake
.Note: Appveyor x86 / Python 3.5 build is failing since this PR generates variable type hints (
x: int = 1
), which are only python 3.6 and up. Do we have any patterns established for skipping tests based on platform/python version?Example difference in generated code after running
thrift --gen py:type_hints
against:ttypes.py
UnderscoreSrv.py
Unrelated to these changes, I noticed that even if a field is marked as required,
declare_argument
generates a default value in the function arguments of None.Open questions: Since the original PR opened,
[skip ci]
anywhere in the commit message to free up build resources.