-
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
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+
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.
Theres really no benefit to | None
besides style. I love that syntax too but this is quite convenient for making it work "always" and without any need to add branches.
@arkuhn According to ASF rules you cannot simply take other peoples code and contribute it., The original author (Konstantin Pozdniakov) can contribute himself. |
Review: document explicit int enum generation requirement instead of implicit Co-authored-by: r/Salomon Smeke <[email protected]>
459be0d
to
eab3abf
Compare
eab3abf
to
7092bd3
Compare
I see @Jens-G. Does this mean we can not add type hints for thrift generated python libs until the original author returns? The original author hasn't commented on the original PR in 2 years. Similarly, if he does return can he include the changes I've made (mostly testing changes, but also functional changes) in his branch or is that prohibited too? I'm happy to accommodate whatever steps are required to get the functionality added. |
2cdc8b7
to
7fd5e4e
Compare
@cspwizard Have you read this? |
Hi! Now I did. Feel free to use my code. If there is anything I can do to help - let me know. There was an issue with my PR about duck typing. I've added some constraints to check the type of the complex property, to prevent garbage (something not defined by the contract) in the serialized message and we couldn't come to agreement on that with maintainers. |
So it seems we have permission from the original contributor, as well as @arkuhn 's changes having its comments addressed. Is all that is missing that this PR be rebased? What can I do to help get this across the line? |
Add type hinting (with 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.