Skip to content
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

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

Merged

Conversation

boukeversteegh
Copy link
Collaborator

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 💪

@boukeversteegh boukeversteegh added bug Something isn't working has test Has a (xfail) test that verifies the bugfix or feature labels Jul 5, 2020
@boukeversteegh boukeversteegh added this to the Better Imports milestone Jul 5, 2020
@boukeversteegh boukeversteegh added the medium Medium effort issue, can fit in a single PR label Jul 8, 2020
nat-n
nat-n previously approved these changes Jul 9, 2020
Copy link
Collaborator

@nat-n nat-n left a comment

Choose a reason for hiding this comment

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

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

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

betterproto/plugin.py Show resolved Hide resolved
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")
Copy link
Collaborator

Choose a reason for hiding this comment

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

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")

@@ -386,6 +428,10 @@ def main():
request = plugin.CodeGeneratorRequest()
request.ParseFromString(data)

dump_file = os.getenv("DUMP_FILE")
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

@@ -373,8 +216,207 @@ def generate_code(request, response):
init = response.file.add()
init.name = str(init_file)

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)):
Copy link
Collaborator

Choose a reason for hiding this comment

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

this diff sucks

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

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


def lookup_method_input_type(method, types):
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

@boukeversteegh boukeversteegh merged commit 8864f4f into danielgtaylor:master Jul 10, 2020
@adriangb
Copy link
Contributor

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).

@boukeversteegh
Copy link
Collaborator Author

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!

@abn abn mentioned this pull request Nov 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working has test Has a (xfail) test that verifies the bugfix or feature medium Medium effort issue, can fit in a single PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Import Bug - No arguments are generated for stub methods when using import with proto definition
3 participants