-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add support for custom message parsing. #76
Conversation
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]>
d1382ae
to
0737f4f
Compare
Signed-off-by: Aidan Jensen <[email protected]>
There was a problem hiding this 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.
So my attempt here was to make a straightforward to use custom message parser, 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. |
There was a problem hiding this 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.
I don't believe I have write permissions on the repo. Happy to wait for 0.1.17 |
Well, I'll get ya'll into develop at least, so I can build upon your commits. Thanks as always for the contributions1 |
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.