Client and Service Stubs take the input type object defined in the proto #311
Conversation
This allows VSCode users to have a devcontainer defined locally
…oto. They no longer pack and unpack the input message fields as kwargs.
Client and Service Stubs take the input type object defined in the proto.
| @@ -17,3 +17,4 @@ output | |||
| .venv | |||
| .asv | |||
| venv | |||
| .devcontainer | |||
There was a problem hiding this comment.
BTW If you'd like well-setup devcontainer for this project with pyenv python 36 and 310 ready to go, lmk and I can make a separate PR.
docs/migrating.rst
Outdated
| @@ -155,3 +155,39 @@ Upgrading: | |||
| - Create a new *empty* directory, e.g. ``generated`` or ``lib/generated/proto`` etc. | |||
| - Regenerate your python files into this directory | |||
| - Update import statements, e.g. ``import ExampleMessage from generated`` | |||
|
|
|||
|
|
|||
| [2.0.0b4] to [2.x.x] | |||
There was a problem hiding this comment.
I wasn't quite sure how to adhere to the format here.
There was a problem hiding this comment.
Hmm... It's a bit of a strange one since it's from one beta release to another.
Personally I would include it in the changelog but I'd like to know what others think.
There was a problem hiding this comment.
Yeah I'm inclined to agree here
There was a problem hiding this comment.
Are you saying move this entire migrating comment in to the changelog, or just delete it? Atm, I put a short line item in the changelog as well.
There was a problem hiding this comment.
I'd suggest just moving most of this to the change log
There was a problem hiding this comment.
08069ba moves the migration notes to the changelog
| str | ||
| Param name corresponding to py_input_message_type. | ||
| """ | ||
| return pythonize_field_name(self.py_input_message_type) |
There was a problem hiding this comment.
I used the type name to drive the input param name, this seemed like it would be a small bonus for anyone not using type hints.
| @@ -111,7 +111,7 @@ omit = ["betterproto/tests/*"] | |||
| legacy_tox_ini = """ | |||
| [tox] | |||
| isolated_build = true | |||
| envlist = py36, py37, py38 | |||
| envlist = py36, py37, py38, py310 | |||
There was a problem hiding this comment.
I noticed you're already running 310 in CI so figured it wouldn't hurt to add to tox, which I then used to test on 3.6 and 3.10
|
I'm happy to approve this but would like other reviewers to weigh in, in case they have any objections I quite like this change, and it brings the library into alignment with other libraries even in other languages |
|
This is great, thanks for doing this! |
|
@kalzoo @nat-n @boukeversteegh shall we await your review or would you be happy to defer to @cetanu and @Gobot1234 on this change? |
There was a problem hiding this comment.
Hi @efokschaner !
Looks like a great improvement, as it makes the generator code more simple, and the usage of the clients more straightforward when you already have messages that can be requests. Type checking will also work better in this way.
As I've indicated in some other issues, I'm not currently working on betterproto, and don't have time to review PR's in depth, so I haven't checked out the code or tested it. The implementation looks decent though, I don't see anything unusual in the code.
I would be in favor of merging this and quickly releasing a new version. If that breaks things for people, we should hear it soon. In that case, I hope you'd be willing to look into it.
|
Thanks @boukeversteegh , I like that plan |
|
Hello. I'm surprised that this change was merged without much discussion. The original behavior (passing parameters to methods instead of message objects) was actually one of the features that we really liked and made us start using I suppose this is a done deal now, but I just wanted to add my opinion for posterity. |
|
@Luminaar There was also discussion on the issue #287 where I think I adequately explained why it was a necessary change and there were no objections. If your message objects are simple it's literally a change of await client.query(a=a, b=b)to await client.query(Message(a=a, b=b))but for more complex objects where construction and destruction are hindrances it makes usage much, much simpler. |
|
@danielgtaylor @efokschaner I just noticed this breaking change and was surprised enough to come here in rush. I think this explains pretty well why the previous design is better. I beg you to reconsider this change. |
This addresses #287
The PR is split in to commits with separated concerns, so take advantage of that for review.
I had a bit of an issue with imports getting sorted automatically. I tried to make sure I was only running black though I may have messed up somewhere. Rather than excluding them all, i stuck them in a dedicated commit: 1010b39
Looking forward to the 2.0 release and your feedback on this PR!