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
c96aab6
to
93d250d
Compare
args not optional type hint UUID generator support Remove runtime type check
9e01199
to
500f732
Compare
500f732
to
05d742d
Compare
It would be great to have this, it would almost enable transpilation via
|
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 ...
@@ -2908,4 +3021,5 @@ THRIFT_REGISTER_GENERATOR( | |||
" Package prefix for generated files.\n" | |||
" old_style: Deprecated. Generate old-style classes.\n" | |||
" enum: Generates Python's IntEnum, connects thrift to python enums. Python 3.4 and higher.\n" | |||
" type_hints: Generate type hints in write method, including IntEnum generation.\n" |
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 would consider re-wording this slightly.
Generate type hints in write method, requires and enables Enum generation.
IntEnum generated files are not backwards compatible with users that leverage the key_to_value mappings to perform conversions.
@@ -127,6 +128,9 @@ class t_py_generator : public t_generator { | |||
gen_tornado_ = true; | |||
} else if( iter->first.compare("coding") == 0) { | |||
coding_ = iter->second; | |||
} else if( iter->first.compare("type_hints") == 0) { | |||
gen_type_hints_ = true; | |||
gen_enum_ = true; |
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.
Would it be worth doing:
if (!gen_enum_) { throw "the type_hints py option requires the enum py option"; }
instead of setting it to true?
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.