Skip to content

[Python] Enhance object API __init__ with typed keyword arguments #8615

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[Python] Enhance object API __init__ with typed keyword arguments #8615

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 93 additions & 18 deletions src/idl_gen_python.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ class PythonStubGenerator {
case BASE_TYPE_STRUCT: {
Import import = imports->Import(ModuleFor(val->union_type.struct_def),
type(*val->union_type.struct_def));
result += import.name;
result += "'" + import.name + "'";
break;
}
case BASE_TYPE_STRING:
Expand Down Expand Up @@ -287,6 +287,62 @@ class PythonStubGenerator {
}
}

void GenerateObjectInitializerStub(std::stringstream &stub,
const StructDef *struct_def,
Imports *imports) const {
stub << " def __init__(self";

for (const FieldDef *field : struct_def->fields.vec) {
if (field->deprecated) continue;

std::string field_name = namer_.Field(*field);
std::string field_type;
const Type &type = field->value.type;

if (IsScalar(type.base_type)) {
field_type = TypeOf(type, imports);
if (field->IsOptional()) { field_type += " | None"; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that the | syntax was introduced in Python 3.10, and since flatbuffers supports Python all the way back to 2.7 should be avoided.

Copy link
Author

Choose a reason for hiding this comment

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

This is a part of .pyi generator. It's already used there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Double checking the documentation for stubs, it just states "all tools support the | syntax", so I expect the syntax is more "python-like" rather than fully python and should be fine.

} else {
switch (type.base_type) {
case BASE_TYPE_STRUCT: {
Import import_ =
imports->Import(ModuleFor(type.struct_def),
namer_.ObjectType(*type.struct_def));
field_type = "'" + import_.name + "' | None";
break;
}
case BASE_TYPE_STRING:
field_type = "str | None";
break;
case BASE_TYPE_ARRAY:
case BASE_TYPE_VECTOR: {
imports->Import("typing");
if (type.element == BASE_TYPE_STRUCT) {
Import import_ =
imports->Import(ModuleFor(type.struct_def),
namer_.ObjectType(*type.struct_def));
field_type = "typing.List['" + import_.name + "'] | None";
} else if (type.element == BASE_TYPE_STRING) {
field_type = "typing.List[str] | None";
} else {
field_type = "typing.List[" + TypeOf(type.VectorType(), imports) +
"] | None";
}
break;
}
case BASE_TYPE_UNION:
field_type = UnionObjectType(*type.enum_def, imports);
break;
default:
field_type = "typing.Any";
break;
}
}
stub << ", " << field_name << ": " << field_type << " = ...";
}
stub << ") -> None: ...\n";
}

void GenerateObjectStub(std::stringstream &stub, const StructDef *struct_def,
Imports *imports) const {
std::string name = namer_.ObjectType(*struct_def);
Expand All @@ -299,6 +355,8 @@ class PythonStubGenerator {
stub << " " << GenerateObjectFieldStub(field, imports) << "\n";
}

GenerateObjectInitializerStub(stub, struct_def, imports);

stub << " @classmethod\n";
stub << " def InitFromBuf(cls, buf: bytes, pos: int) -> " << name
<< ": ...\n";
Expand Down Expand Up @@ -1674,6 +1732,7 @@ class PythonGenerator : public BaseGenerator {
field_type = package_reference + "." + field_type;
import_list->insert("import " + package_reference);
}
field_type = "'" + field_type + "'";
break;
case BASE_TYPE_STRING: field_type += "str"; break;
case BASE_TYPE_NONE: field_type += "None"; break;
Expand All @@ -1694,20 +1753,17 @@ class PythonGenerator : public BaseGenerator {
}

void GenStructInit(const FieldDef &field, std::string *out_ptr,
std::set<std::string> *import_list,
std::set<std::string> *import_typing_list) const {
import_typing_list->insert("Optional");
std::set<std::string> *import_list) const {
auto &output = *out_ptr;
const Type &type = field.value.type;
const std::string object_type = namer_.ObjectType(*type.struct_def);
if (parser_.opts.include_dependence_headers) {
auto package_reference = GenPackageReference(type);
output = package_reference + "." + object_type + "]";
output = "'" + package_reference + "." + object_type + "'";
import_list->insert("import " + package_reference);
} else {
output = object_type + "]";
output = "'" + object_type + "'";
}
output = "Optional[" + output;
}

void GenVectorInit(const FieldDef &field, std::string *field_type_ptr,
Expand All @@ -1720,13 +1776,15 @@ class PythonGenerator : public BaseGenerator {
if (base_type == BASE_TYPE_STRUCT) {
const std::string object_type =
namer_.ObjectType(*vector_type.struct_def);
field_type = object_type + "]";
std::string full_object_type;
if (parser_.opts.include_dependence_headers) {
auto package_reference = GenPackageReference(vector_type);
field_type = package_reference + "." + object_type + "]";
full_object_type = package_reference + "." + object_type;
import_list->insert("import " + package_reference);
} else {
full_object_type = object_type;
}
field_type = "List[" + field_type;
field_type = "List['" + full_object_type + "']";
} else {
field_type =
"List[" + GetBasePythonTypeForScalarAndString(base_type) + "]";
Expand All @@ -1735,8 +1793,10 @@ class PythonGenerator : public BaseGenerator {

void GenInitialize(const StructDef &struct_def, std::string *code_ptr,
std::set<std::string> *import_list) const {
std::string code;
std::string signature_params;
std::string init_body;
std::set<std::string> import_typing_list;

for (auto it = struct_def.fields.vec.begin();
it != struct_def.fields.vec.end(); ++it) {
auto &field = **it;
Expand All @@ -1751,7 +1811,7 @@ class PythonGenerator : public BaseGenerator {
break;
}
case BASE_TYPE_STRUCT: {
GenStructInit(field, &field_type, import_list, &import_typing_list);
GenStructInit(field, &field_type, import_list);
break;
}
case BASE_TYPE_VECTOR:
Expand All @@ -1763,26 +1823,41 @@ class PythonGenerator : public BaseGenerator {
// Scalar or sting fields.
field_type = GetBasePythonTypeForScalarAndString(base_type);
if (field.IsScalarOptional()) {
import_typing_list.insert("Optional");
field_type = "Optional[" + field_type + "]";
}
break;
}

const auto default_value = GetDefaultValue(field);
// Wrties the init statement.
const auto field_field = namer_.Field(field);
code += GenIndents(2) + "self." + field_field + " = " + default_value +
" # type: " + field_type;

// If the default value is None, the type must be Optional, unless it's a
// Union that already contains None.
const bool is_already_optional =
(field_type.rfind("Optional[", 0) == 0) ||
(field_type.rfind("Union[", 0) == 0);
if (default_value == "None" && !is_already_optional) {
import_typing_list.insert("Optional");
field_type = "Optional[" + field_type + "]";
}

// Build signature with keyword arguments, type hints, and default values.
signature_params +=
", " + field_field + ": " + field_type + " = " + default_value;
Copy link
Contributor

Choose a reason for hiding this comment

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

Typing is opt-in with --python-typing, no typing imports or annotations should be output if not explicitly requested.

Copy link
Contributor

Choose a reason for hiding this comment

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

also the type info should end up in the .pyi files rather than the py code directly

Copy link
Contributor

Choose a reason for hiding this comment

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

Best I can tell when looking at the generator code, when --python-typing is specified it will output at least some typing annotations in the .py file.

Copy link
Author

Choose a reason for hiding this comment

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

What is your suggestion? Should we keep the old behavior (self.a = a # type: float) and no typed __init__, and defer all typing to .pyi, or old typing behavior (comments) if no --python-typing and new if enabled?

Copy link
Contributor

Choose a reason for hiding this comment

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

The generated code seems to be really inconsistent when type info is output, so it's hard to make a good suggestion.

When looking at the MonsterExtra.py test output for an example, it seems like the primary API has at least some (but not complete) type info, while the newer object API at the end created with --gen-object-api relegates all typing to the .pyi stub file.

Based on that, as this adds to the newer object API, I think that means that the typing should be removed from the .py file and existing comments for the types should probably stay. Realistically this should be made more consistent, such as moving the original non-object API typing info to the stub files, but as far as this incremental change that makes the most sense to me.

BTW it may be worth putting each parameter on a separate line for the generated __init__ functions, otherwise larger tables can horizontally scroll to infinity.


// Build the body of the __init__ method.
init_body += GenIndents(2) + "self." + field_field + " = " + field_field;
}

// Writes __init__ method.
auto &code_base = *code_ptr;
GenReceiverForObjectAPI(struct_def, code_ptr);
code_base += "__init__(self):";
if (code.empty()) {
code_base += "__init__(self" + signature_params + "):";
if (init_body.empty()) {
code_base += GenIndents(2) + "pass";
} else {
code_base += code;
code_base += init_body;
}
code_base += "\n";

Expand Down
6 changes: 3 additions & 3 deletions tests/MyGame/Example/Ability.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,9 @@ def CreateAbility(builder, id, distance):
class AbilityT(object):

# AbilityT
def __init__(self):
self.id = 0 # type: int
self.distance = 0 # type: int
def __init__(self, id: int = 0, distance: int = 0):
self.id = id
self.distance = distance

@classmethod
def InitFromBuf(cls, buf, pos):
Expand Down
16 changes: 8 additions & 8 deletions tests/MyGame/Example/ArrayStruct.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,20 +107,20 @@ def CreateArrayStruct(builder, a, b, c, d_a, d_b, d_c, d_d, e, f):

import MyGame.Example.NestedStruct
try:
from typing import List
from typing import List, Optional
except:
pass

class ArrayStructT(object):

# ArrayStructT
def __init__(self):
self.a = 0.0 # type: float
self.b = None # type: List[int]
self.c = 0 # type: int
self.d = None # type: List[MyGame.Example.NestedStruct.NestedStructT]
self.e = 0 # type: int
self.f = None # type: List[int]
def __init__(self, a: float = 0.0, b: Optional[List[int]] = None, c: int = 0, d: Optional[List['MyGame.Example.NestedStruct.NestedStructT']] = None, e: int = 0, f: Optional[List[int]] = None):
self.a = a
self.b = b
self.c = c
self.d = d
self.e = e
self.f = f

@classmethod
def InitFromBuf(cls, buf, pos):
Expand Down
1 change: 1 addition & 0 deletions tests/MyGame/Example/ArrayStruct.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ class ArrayStructT(object):
d: typing.List[NestedStructT]
e: int
f: typing.List[int]
def __init__(self, a: float = ..., b: typing.List[int] | None = ..., c: int = ..., d: typing.List['NestedStructT'] | None = ..., e: int = ..., f: typing.List[int] | None = ...) -> None: ...
@classmethod
def InitFromBuf(cls, buf: bytes, pos: int) -> ArrayStructT: ...
@classmethod
Expand Down
4 changes: 2 additions & 2 deletions tests/MyGame/Example/ArrayTable.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,8 @@ def End(builder: flatbuffers.Builder) -> int:
class ArrayTableT(object):

# ArrayTableT
def __init__(self):
self.a = None # type: Optional[MyGame.Example.ArrayStruct.ArrayStructT]
def __init__(self, a: Optional['MyGame.Example.ArrayStruct.ArrayStructT'] = None):
self.a = a

@classmethod
def InitFromBuf(cls, buf, pos):
Expand Down
1 change: 1 addition & 0 deletions tests/MyGame/Example/ArrayTable.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ class ArrayTable(object):
def A(self) -> ArrayStruct | None: ...
class ArrayTableT(object):
a: ArrayStructT | None
def __init__(self, a: 'ArrayStructT' | None = ...) -> None: ...
@classmethod
def InitFromBuf(cls, buf: bytes, pos: int) -> ArrayTableT: ...
@classmethod
Expand Down
Loading