[Draft] Fix many import related bugs #47

boukeversteegh · 2020-05-21T09:16:57Z

Fixes lots of issues with imports.

Circular imports

two python files referring to eachother, preventing imports

I've solved this by wrapping generated Types in quotes (List["Foo"])

Imports from parent package

I've solved this for now by using absolute imports there, but I think this still needs a change to use from ... import [parent] instead.

Module and filename conflicts

consider two messages animal.domestic.cow, and animal.bird which is in the animal.proto file. the python files would look like

animal/domestic/cow.py
animal.py

that means animal is now both a file and a module, and it results in conflicts.

I've solved this by exporting all packages as modules:

animal/
   __init__.py  # contains bird
   domestic/
     __init__.py  # contains cow

You can import the animal package simply with import animal as before, but still also import animal.domestic.

import name conflicts

Importing two subpackages with the same name resulted in conflicts. See: #25

Solved this by always aliasing the imported package with full package path:

import from .aedt.method_data import v0 as aedt_method_data_v0

relative imports

Relative imports work correctly for siblings, children and descendants.

… application

…ents, module and filename conflicts, import name conflicts, relative imports

…itted from output

nat-n · 2020-05-21T13:37:45Z

This looks good (I'm still reading).

Perhaps rounding up issues of this nature would also fit well within the scope of this change: #37

nat-n

Don't forget to run black.

nat-n · 2020-05-22T14:00:42Z

betterproto/plugin.py

-                    )
-
-                    output["enums"].append(data)
+                read_item(item, path, proto_file, output_file_data['package'], output_file_data['template_data'], global_messages)


black would have something to say about the line length here.

nat-n · 2020-05-22T14:13:32Z

betterproto/plugin.py

+    for output_file_name, output_file_data in output_files.items():
+        package = output_file_data["package"]
+        template_data = {
            "package": package,
-            "files": [f.name for f in options["files"]],
+            "files": [f.name for f in output_file_data["files"]],
            "imports": set(),
            "datetime_imports": set(),
            "typing_imports": set(),
            "messages": [],
            "enums": [],
            "services": [],
        }
+        output_file_data['template_data'] = template_data

-        type_mapping = {}
-
-        for proto_file in options["files"]:
-            # print(proto_file.message_type, file=sys.stderr)
-            # print(proto_file.service, file=sys.stderr)
-            # print(proto_file.source_code_info, file=sys.stderr)
-
+    # read messages
+    global_messages = []
+    for output_file_name, output_file_data in output_files.items():
+        for proto_file in output_file_data["files"]:


this bit could be streamlined a bit more since you're here:

global_messages = [] for _, output_file_data in output_files.items(): output_file_data['template_data'] = { "package": output_file_data["package"], "files": [f.name for f in output_file_data["files"]], "imports": set(), "datetime_imports": set(), "typing_imports": set(), "messages": [], "enums": [], "services": [], } # read messages for proto_file in output_file_data["files"]: for item, path in traverse(proto_file): read_item( item, path, proto_file, # < No need to pass packages seperately becuase it's in template_data > output_file_data['template_data'], global_messages )

nat-n · 2020-05-22T14:22:37Z

betterproto/plugin.py

+                one_of = item.oneof_decl[field.oneof_index].name
+
+            if "Optional[" in t:
+                template_data["typing_imports"].add("Optional")
+
+            if "timedelta" in t:
+                template_data["datetime_imports"].add("timedelta")
+            elif "datetime" in t:
+                template_data["datetime_imports"].add("datetime")
+
+            item_data["properties"].append(
+                {
+                    "name": field.name,
+                    "py_name": safe_snake_case(field.name),
+                    "number": field.number,
+                    "comment": get_comment(proto_file, path + [2, i]),
+                    "proto_type": int(field.type),
+                    "field_type": field_type,
+                    "field_wraps": field_wraps,
+                    "map_types": map_types,
+                    "type": t,
+                    "zero": zero,
+                    "repeated": repeated,
+                    "packed": packed,
+                    "one_of": one_of,
+                }
+            )
+
+        template_data["messages"].append(item_data)
+        global_messages.append(item_data)
+    elif isinstance(item, EnumDescriptorProto):
+        item_data.update(
+            {
+                "type": "Enum",
+                "comment": get_comment(proto_file, path),
+                "entries": [
+                    {
+                        "name": v.name,
+                        "value": v.number,
+                        "comment": get_comment(proto_file, path + [2, i]),
+                    }
+                    for i, v in enumerate(item.value)
+                ],
+            }
+        )
+        template_data["enums"].append(item_data)
+
+
+def read_service(i, package, proto_file, service, template_data, global_messages):
+    data = {
+        "name": service.name,
+        "py_name": stringcase.pascalcase(service.name),
+        "comment": get_comment(proto_file, [6, i]),
+        "methods": [],
+    }
+    for j, method in enumerate(service.method):
+        if method.client_streaming:
+            raise NotImplementedError("Client streaming not yet supported")
+        sys.stderr.write(f'  add {method.name}\n')
+
+        input_message = None
+        input_type = get_ref_type(
+            package, template_data["imports"], method.input_type
+        ).strip('"')
+        for msg in global_messages:
+            if msg["full_name"] == method.input_type.lstrip('.'):
+                input_message = msg
+                for field in msg["properties"]:
+                    if field["zero"] == "None":
+                        template_data["typing_imports"].add("Optional")
+                break
+
+        if not input_message:
+            sys.stderr.write(f"no input message found for {input_type} / {method}\n")
+            sys.stderr.write(f" {global_messages}\n")
+            sys.stderr.write(f"\n")
+
+        method_data = {
+            "name": method.name,
+            "py_name": stringcase.snakecase(method.name),
+            "comment": get_comment(proto_file, [6, i, 2, j], indent=8),
+            "route": f"/{package}.{service.name}/{method.name}",
+            "input": get_ref_type(
+                package, template_data["imports"], method.input_type
+            ).strip('"'),
+            "input_message": input_message,
+            "output": get_ref_type(
+                package, template_data["imports"], method.output_type, unwrap=False
+            ).strip('"'),
+            "client_streaming": method.client_streaming,
+            "server_streaming": method.server_streaming,
+        }
+        # sys.stderr.write(f'METHOD: {method_data}\n')
+        data["methods"].append(method_data)
+
+        if method.server_streaming:
+            template_data["typing_imports"].add("AsyncGenerator")
+    template_data["services"].append(data)
+
+


The diff makes it look like this is all new. Is it fair to assume none of it really is?

Yes. Its because I've moved some code into functions, and the changed indentation has made it look new.

However, I did make an important change on line 427

if msg["full_name"] == method.input_type.lstrip('.'):

to compare with the full name, rather than part of the name, so that all messages can be matched consistently.

boukeversteegh added 8 commits May 10, 2020 13:03

Fixes issue where protoc did not recognize plugin.py as a valid win32…

82930e1

… application

Support using Google's wrapper types as RPC output values

499489f

Support using plugin.bat outside of tests directory

20454f5

Merge branch 'pr/wrapper-as-output' into dev

7236697

Fixes issue where packages with a common prefix could not be imported

86628d2

Merge branch 'fix/package-prefix-collisson' into dev

2ebf73a

Fixes lots of issues with imports. Circular imports, imports from par…

aa3f7c7

…ents, module and filename conflicts, import name conflicts, relative imports

Fixes issue where input messages imported from other packages were om…

5f813bf

…itted from output

boukeversteegh marked this pull request as draft May 21, 2020 09:52

boukeversteegh mentioned this pull request May 22, 2020

Organize test-cases #51

Merged

nat-n reviewed May 22, 2020

View reviewed changes

This was referenced May 22, 2020

Split code up into multiple modules #49

Open

Support using Google's wrapper types as RPC output values #40

Merged

boukeversteegh closed this May 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Draft] Fix many import related bugs #47

[Draft] Fix many import related bugs #47

boukeversteegh commented May 21, 2020

nat-n commented May 21, 2020 •

edited

Loading

nat-n left a comment

nat-n May 22, 2020

nat-n May 22, 2020

nat-n May 22, 2020

boukeversteegh May 22, 2020

[Draft] Fix many import related bugs #47

[Draft] Fix many import related bugs #47

Conversation

boukeversteegh commented May 21, 2020

Circular imports

Imports from parent package

Module and filename conflicts

import name conflicts

relative imports

nat-n commented May 21, 2020 • edited Loading

nat-n left a comment

Choose a reason for hiding this comment

nat-n May 22, 2020

Choose a reason for hiding this comment

nat-n May 22, 2020

Choose a reason for hiding this comment

nat-n May 22, 2020

Choose a reason for hiding this comment

boukeversteegh May 22, 2020

Choose a reason for hiding this comment

nat-n commented May 21, 2020 •

edited

Loading