Fix - No arguments are generated for stub methods when using `import` with proto definition by boukeversteegh · Pull Request #103 · danielgtaylor/python-betterproto · GitHub

Fixes #23

Due to too much juggling with different branches, and an incorrect test-case, I mistakenly thought this bug was already fixed earlier.

The bug:

Define a Service with an rpc method that has a Message argument
Betterproto will unpack the message fields, and add them as arguments directly to the generated client method
While compiling, we need to lookup the correct message that acts as the rpc method input argument
If the Service was compiled before the Message, it cannot be found, and input arguments are skipped.
The algorithm to match the Service Method Input type with the correct message was also shaky and did not work at all when the arguments were nested or in packages

The solution

First read and compile all the Messages
Then compile all the Services
Match the method input types more precisely by keeping track of the parsed message' packages

Additionally:

It was very hard to debug plugin.py as it is called by protoc, so attaching a debugger involves freezing the plugin when it starts, then finding the process id, then attaching, and waiting till the freeze is completed.
- To make life better, I've added a way to write the input provided by protoc to a file, and then run plugin.py directly with that input (stdin). Now you can debug the plugin with your IDE 💪

… is flattened into two python packages

… its possible to debug it.

…s fixed before.

…essage # Conflicts: # betterproto/plugin.py

Looks good. Multiple passes is definitely a step in the right direction.

I'd say it's good to go with a couple tweaks.

Suggested change

      
                    sys.stderr.write(f"\033[31mWriting: {dump_file}\033[0m\n")
          
                    sys.stderr.write(f"\033[31mWriting input from protoc to: {dump_file}\033[0m\n")

Suggested change

      
                dump_file = os.getenv("DUMP_FILE")
          
                dump_file = os.getenv("BETTERPROTO_DUMP")

this diff sucks

yeah, it went pretty bad after splitting up code into methods and renaming a bunch of vars :-/

should happen less and less as we get the code more organized

could move this method closer to where it's used (swap with read_protobuf_type)

Just a quick note about debugging: I had good success using VSCode with ptsvd.

In plugin.py:

def main():

    import ptvsd
    ptvsd.enable_attach()
    ptvsd.wait_for_attach()  # blocks execution until debugger is attached

And set breakpoints in VSCode wherever you want to drop into.

Then in launch.json:

{
    "version": "0.2.0",
    "configurations": [

        {
            "name": "Python: Current File",
            "type": "python",
            "request": "launch",
            "program": "${file}",
            "console": "integratedTerminal",
            "justMyCode": false,
        },
        {
            "name": "Attach (Remote Debug)",
            "type": "python",
            "request": "attach",
            "localRoot": "${workspaceRoot}",
            "remoteRoot": "${workspaceRoot}",
            "port": 5678,
            "secret": "",
            "host":"0.0.0.0",
            "justMyCode": false,
        },
    ]
}

Now running anything that calls plugin.py will just wait until you attach the VSCode debugger before continuing (and stopping at your breakpoints).

Just a quick note about debugging: I had good success using VSCode with ptsvd.

Awesome tip! I took a note when you mentioned it on slack, and also found that PyCharm has something similar for those who use it. Haven't tried it yet tho!

boukeversteegh added 6 commits July 4, 2020 15:35

Fixes circular import problem when a non-circular dependency triangle…

0af0cf4

… is flattened into two python packages

Supports running plugin.py standalone by reading from a dump-file, so…

98d00f0

… its possible to debug it.

Clarify variable names

f2e8719

Move parsing of protobuf data types and services into separate methods

87b3a4b

Read proto objects before services

dedead0

Fixes danielgtaylor#23 again, a broken test made it seem the issue wa…

3f519d4

…s fixed before.

boukeversteegh added bug Something isn't working has test Has a (xfail) test that verifies the bugfix or feature labels Jul 5, 2020

boukeversteegh added this to the Better Imports milestone Jul 5, 2020

Merge remote-tracking branch 'daniel/master' into fix/service-input-m…

72d72b4

…essage # Conflicts: # betterproto/plugin.py

boukeversteegh added the medium Medium effort issue, can fit in a single PR label Jul 8, 2020

nat-n previously approved these changes Jul 9, 2020

View reviewed changes

Reorder methods, use BETTERPROTO_DUMP for dump env var, docs.

1d7ba85

boukeversteegh dismissed nat-n’s stale review via 1d7ba85 July 9, 2020 21:09

nat-n approved these changes Jul 10, 2020

View reviewed changes

boukeversteegh merged commit 8864f4f into danielgtaylor:master Jul 10, 2020

abn mentioned this pull request Nov 24, 2020

Release v2.0.0b2 #175

Merged

+                  Then run plugin.py from your IDE in debugging mode, and redirect stdin to the file.
+                  """
+                  with open(str(dump_file), "wb") as fh:
+                      sys.stderr.write(f"\033[31mWriting: {dump_file}\033[0m\n")

Fix - No arguments are generated for stub methods when using `import` with proto definition by boukeversteegh · Pull Request #103 · danielgtaylor/python-betterproto · GitHub

Fix - No arguments are generated for stub methods when using import with proto definition - #103

Fix - No arguments are generated for stub methods when using import with proto definition#103
boukeversteegh merged 8 commits into
danielgtaylor:masterfrom
boukeversteegh:fix/service-input-message

boukeversteegh commented Jul 5, 2020

Uh oh!

nat-n left a comment

Uh oh!

Uh oh!

nat-n Jul 9, 2020

Uh oh!

nat-n Jul 9, 2020

Uh oh!

nat-n Jul 9, 2020

Uh oh!

boukeversteegh Jul 9, 2020

Uh oh!

nat-n Jul 9, 2020

Uh oh!

adriangb commented Jul 15, 2020

Uh oh!

boukeversteegh commented Jul 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	sys.stderr.write(f"\033[31mWriting: {dump_file}\033[0m\n")
	sys.stderr.write(f"\033[31mWriting input from protoc to: {dump_file}\033[0m\n")

                   request = plugin.CodeGeneratorRequest()
                   request.ParseFromString(data)
+                  dump_file = os.getenv("DUMP_FILE")

	dump_file = os.getenv("DUMP_FILE")
	dump_file = os.getenv("BETTERPROTO_DUMP")

-                  for filename in sorted(output_paths.union(init_files)):
-                      print(f"Writing {filename}", file=sys.stderr)
+                  for output_package_name in sorted(output_paths.union(init_files)):

		print(f"Writing {output_package_name}", file=sys.stderr)


		def lookup_method_input_type(method, types):

Fix - No arguments are generated for stub methods when using `import` with proto definition by boukeversteegh · Pull Request #103 · danielgtaylor/python-betterproto · GitHub

Fix - No arguments are generated for stub methods when using import with proto definition - #103

Conversation

boukeversteegh commented Jul 5, 2020

Uh oh!

nat-n left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nat-n Jul 9, 2020

Choose a reason for hiding this comment

Uh oh!

nat-n Jul 9, 2020

Choose a reason for hiding this comment

Uh oh!

nat-n Jul 9, 2020

Choose a reason for hiding this comment

Uh oh!

boukeversteegh Jul 9, 2020

Choose a reason for hiding this comment

Uh oh!

nat-n Jul 9, 2020

Choose a reason for hiding this comment

Uh oh!

adriangb commented Jul 15, 2020

Uh oh!

boukeversteegh commented Jul 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants