You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by GitBox <gi...@apache.org> on 2022/02/16 13:50:41 UTC
[GitHub] [thrift] cspwizard opened a new pull request #2523: THRIFT-5139: THRIFT-4181: Python3 type hints
cspwizard opened a new pull request #2523:
URL: https://github.com/apache/thrift/pull/2523
Add type hinting and native enums (IntEnum) for python generated code
<!-- We recommend you review the checklist/tips before submitting a pull request. -->
- [] Did you create an [Apache Jira](https://issues.apache.org/jira/projects/THRIFT/issues/) ticket? (not required for trivial changes)
- [X] If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
- [X] Did you squash your changes to a single commit? (not required, but preferred)
- [X] Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
- [ ] If your change does not involve any code, include `[skip ci]` anywhere in the commit message to free up build resources.
<!--
The Contributing Guide at:
https://github.com/apache/thrift/blob/master/CONTRIBUTING.md
has more details and tips for committing properly.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] jeking3 commented on a change in pull request #2523: THRIFT-5139: THRIFT-4181: Python3 type hints
Posted by GitBox <gi...@apache.org>.
jeking3 commented on a change in pull request #2523:
URL: https://github.com/apache/thrift/pull/2523#discussion_r841067092
##########
File path: compiler/cpp/src/thrift/generate/t_py_generator.cc
##########
@@ -2497,6 +2513,11 @@ void t_py_generator::generate_serialize_field(ostream& out, t_field* tfield, str
* @param prefix String prefix to attach to all fields
*/
void t_py_generator::generate_serialize_struct(ostream& out, t_struct* tstruct, string prefix) {
+ if(gen_type_hints_) {
+ indent(out) << "if not type(" << prefix << ") is " << type_to_py_type(tstruct)
Review comment:
This is not a type hint, it is a run-time check. I don't think it belongs in this set of changes. This is likely to break code relying on duck typing such as mocking.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G commented on pull request #2523: THRIFT-5139: THRIFT-4181: Python3 type hints
Posted by GitBox <gi...@apache.org>.
Jens-G commented on pull request #2523:
URL: https://github.com/apache/thrift/pull/2523#issuecomment-1059950144
If there is a reason, leave it. I just want to make sure we don't introduce unnecessary switches. Seems not the case. As I said, stupid question :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G commented on pull request #2523: THRIFT-5139: THRIFT-4181: Python3 type hints
Posted by GitBox <gi...@apache.org>.
Jens-G commented on pull request #2523:
URL: https://github.com/apache/thrift/pull/2523#issuecomment-1059948340
Stupid question (probably): Can we make it the default or what reason does the switch have? Are there any potential incompatibilities? Just asking.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] cspwizard commented on a change in pull request #2523: THRIFT-5139: THRIFT-4181: Python3 type hints
Posted by GitBox <gi...@apache.org>.
cspwizard commented on a change in pull request #2523:
URL: https://github.com/apache/thrift/pull/2523#discussion_r825304780
##########
File path: compiler/cpp/src/thrift/generate/t_py_generator.cc
##########
@@ -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;
Review comment:
added it, but it should be obvious as without enum type type hinting won't work.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] kainjow commented on a change in pull request #2523: THRIFT-5139: THRIFT-4181: Python3 type hints
Posted by GitBox <gi...@apache.org>.
kainjow commented on a change in pull request #2523:
URL: https://github.com/apache/thrift/pull/2523#discussion_r825244642
##########
File path: compiler/cpp/src/thrift/generate/t_py_generator.cc
##########
@@ -2742,8 +2765,73 @@ string t_py_generator::type_name(t_type* ttype) {
return ttype->get_name();
}
+string t_py_generator::arg_hint(t_type* type) {
+ if(gen_type_hints_) {
+ return ": " + type_to_py_hint(type);
+ }
+
+ return "";
+}
+
+string t_py_generator::func_hint(t_type* type) {
+ if(gen_type_hints_) {
+ return " -> " + type_to_py_hint(type);
+ }
+
+ return "";
+}
+
+/**
+ * Converts the parse type to a Python type hint
+ */
+string t_py_generator::type_to_py_hint(t_type* type) {
+ return "typing.Optional[" + type_to_py_type(type) + "]";
+}
+
+/**
+ * Converts the parse type to a Python type
+ */
+string t_py_generator::type_to_py_type(t_type* type) {
+ type = get_true_type(type);
+
+ if (type->is_binary()) {
+ return "bytes";
+ }
+ if (type->is_base_type()) {
+ t_base_type::t_base tbase = ((t_base_type*)type)->get_base();
+ switch (tbase) {
+ case t_base_type::TYPE_VOID:
+ return "None";
+ case t_base_type::TYPE_STRING:
+ return "str";
+ case t_base_type::TYPE_BOOL:
+ return "bool";
+ case t_base_type::TYPE_I8:
+ case t_base_type::TYPE_I16:
+ case t_base_type::TYPE_I32:
+ case t_base_type::TYPE_I64:
+ return "int";
+ case t_base_type::TYPE_DOUBLE:
+ return "float";
+ }
+ } else if (type->is_enum()) {
Review comment:
could this be combined with the check below since they both return the same value?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] kainjow commented on a change in pull request #2523: THRIFT-5139: THRIFT-4181: Python3 type hints
Posted by GitBox <gi...@apache.org>.
kainjow commented on a change in pull request #2523:
URL: https://github.com/apache/thrift/pull/2523#discussion_r825244144
##########
File path: compiler/cpp/src/thrift/generate/t_py_generator.cc
##########
@@ -2839,4 +2927,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 and type checks in write method.\n"
Review comment:
nit: can you make the spacing line up?
```suggestion
" type_hints: Generate type hints and type checks in write method.\n"
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] kainjow commented on a change in pull request #2523: THRIFT-5139: THRIFT-4181: Python3 type hints
Posted by GitBox <gi...@apache.org>.
kainjow commented on a change in pull request #2523:
URL: https://github.com/apache/thrift/pull/2523#discussion_r825244260
##########
File path: compiler/cpp/src/thrift/generate/t_py_generator.cc
##########
@@ -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;
Review comment:
should the help text below mention that the enum option is also enabled if this option is enabled?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] cspwizard commented on pull request #2523: THRIFT-5139: THRIFT-4181: Python3 type hints
Posted by GitBox <gi...@apache.org>.
cspwizard commented on pull request #2523:
URL: https://github.com/apache/thrift/pull/2523#issuecomment-1059949929
Just in case if somebody use lower versions of Python. However it make sense to make it default, and for older versions users can use the previous version. For me both ways are good, what is your opinion? @Jens-G
I'll rebase
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G edited a comment on pull request #2523: THRIFT-5139: THRIFT-4181: Python3 type hints
Posted by GitBox <gi...@apache.org>.
Jens-G edited a comment on pull request #2523:
URL: https://github.com/apache/thrift/pull/2523#issuecomment-1059948340
Stupid question (probably): Can we make it the default or what reason does the switch have? Are there any potential incompatibilities? Just asking.
PS: you should rebase, theres's a conflict
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] kainjow commented on pull request #2523: THRIFT-5139: THRIFT-4181: Python3 type hints
Posted by GitBox <gi...@apache.org>.
kainjow commented on pull request #2523:
URL: https://github.com/apache/thrift/pull/2523#issuecomment-1046040636
Cool! There's a PR here for enum changes: #2489
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] cspwizard commented on pull request #2523: THRIFT-5139: THRIFT-4181: Python3 type hints
Posted by GitBox <gi...@apache.org>.
cspwizard commented on pull request #2523:
URL: https://github.com/apache/thrift/pull/2523#issuecomment-1059966432
rebased and fixed a bug, for some reason on de/serialization it used enum variable as string
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] jeking3 commented on a change in pull request #2523: THRIFT-5139: THRIFT-4181: Python3 type hints
Posted by GitBox <gi...@apache.org>.
jeking3 commented on a change in pull request #2523:
URL: https://github.com/apache/thrift/pull/2523#discussion_r841066871
##########
File path: compiler/cpp/src/thrift/generate/t_py_generator.cc
##########
@@ -2742,8 +2765,71 @@ string t_py_generator::type_name(t_type* ttype) {
return ttype->get_name();
}
+string t_py_generator::arg_hint(t_type* type) {
+ if(gen_type_hints_) {
+ return ": " + type_to_py_hint(type);
+ }
+
+ return "";
+}
+
+string t_py_generator::func_hint(t_type* type) {
+ if(gen_type_hints_) {
+ return " -> " + type_to_py_hint(type);
+ }
+
+ return "";
+}
+
+/**
+ * Converts the parse type to a Python type hint
+ */
+string t_py_generator::type_to_py_hint(t_type* type) {
+ return "typing.Optional[" + type_to_py_type(type) + "]";
+}
+
+/**
+ * Converts the parse type to a Python type
+ */
+string t_py_generator::type_to_py_type(t_type* type) {
+ type = get_true_type(type);
+
+ if (type->is_binary()) {
+ return "bytes";
+ }
+ if (type->is_base_type()) {
+ t_base_type::t_base tbase = ((t_base_type*)type)->get_base();
+ switch (tbase) {
+ case t_base_type::TYPE_VOID:
+ return "None";
+ case t_base_type::TYPE_STRING:
+ return "str";
+ case t_base_type::TYPE_BOOL:
+ return "bool";
+ case t_base_type::TYPE_I8:
+ case t_base_type::TYPE_I16:
+ case t_base_type::TYPE_I32:
+ case t_base_type::TYPE_I64:
+ return "int";
+ case t_base_type::TYPE_DOUBLE:
+ return "float";
+ }
+ } else if (type->is_enum() || type->is_struct() || type->is_xception()) {
+ return type_name(type);
+ } else if (type->is_map()) {
+ return "dict[" + type_to_py_type(((t_map*)type)->get_key_type()) + ", " + type_to_py_type(((t_map*)type)->get_val_type()) + "]";
Review comment:
This should be using `Dict[...]` and `from typing import Dict` ?
Same below for `Set` and `List` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org