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

Add support for custom message parsing. #76

Merged

Conversation

artificial-aidan
Copy link

Ran into an issue where we needed control of the message parsing functionality. Specifically the ability to include default field values. Set it up so that it works the same as default, but a custom parsing class can be passed in.

Just did the async side for now, can port to the sync side if wanted.

Test added to override default `MessageToDict` options

Signed-off-by: Aidan Jensen <[email protected]>
Signed-off-by: Aidan Jensen <[email protected]>
Signed-off-by: Aidan Jensen <[email protected]>
Signed-off-by: Aidan Jensen <[email protected]>
@ViridianForge ViridianForge self-requested a review April 15, 2024 01:56
@ViridianForge ViridianForge self-assigned this Apr 15, 2024
Copy link
Collaborator

@ViridianForge ViridianForge left a comment

Choose a reason for hiding this comment

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

One question about the implementation, but the spirit looks good.

Do you have a gut feel on whether there may be numerous different MessageProtocol implementation the library may want to support in the future?

I ask, because the client classes are feeling a bit hefty already, and it may be worthwhile to start considering breaking the code apart a bit in the near future.

src/grpc_requests/aio.py Show resolved Hide resolved
@artificial-aidan
Copy link
Author

One question about the implementation, but the spirit looks good.

Do you have a gut feel on whether there may be numerous different MessageProtocol implementation the library may want to support in the future?

I ask, because the client classes are feeling a bit hefty already, and it may be worthwhile to start considering breaking the code apart a bit in the near future.

So my attempt here was to make a straightforward to use custom message parser, CustomArgumentParsers. Which lets you set the keyword args for MessageToDict, while also allowing someone to expand if they wanted to do something completely different (json grpc or something) without having to change the library.

So 90% of people probably won't use custom parsers, 9% of people will use the built in custom parser with custom arguments, and 1% can write their own parser implementation.

Copy link
Collaborator

@ViridianForge ViridianForge left a comment

Choose a reason for hiding this comment

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

Thanks for answering my questions. All in all looks good - feel free to merge at your leisure, or, I'll go ahead and merge it in when I get some time later this week to work on a new release, as we slated those deprecated methods to be removed in 0.1.17.

@artificial-aidan
Copy link
Author

Thanks for answering my questions. All in all looks good - feel free to merge at your leisure, or, I'll go ahead and merge it in when I get some time later this week to work on a new release, as we slated those deprecated methods to be removed in 0.1.17.

I don't believe I have write permissions on the repo. Happy to wait for 0.1.17

@ViridianForge
Copy link
Collaborator

Well, I'll get ya'll into develop at least, so I can build upon your commits. Thanks as always for the contributions1

@ViridianForge ViridianForge merged commit 0731b5d into grpc-requests:develop Apr 16, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants