-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Implement methods to convert a Struct object to a pythonic object #1951
Conversation
kafka/protocol/api.py
Outdated
def _to_object(schema, data): | ||
obj = {} | ||
for idx, (name, _type) in enumerate(zip(schema.names, schema.fields)): | ||
if isinstance(data, dict): |
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.
why not check for Struct
and use the existing attributes set in __dict__
, as opposed to creating a duplicate dictionary for the same data (struct.fields) ?
kafka/protocol/struct.py
Outdated
@@ -12,14 +12,17 @@ class Struct(AbstractType): | |||
SCHEMA = Schema() | |||
|
|||
def __init__(self, *args, **kwargs): | |||
self.fields = {} |
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.
I don't think we need this -- the same data is in __dict__
and could be accessed via getattr
. Or you might add a helper, like get_field(name)
that returns __dict__[name]
I like it! Just a few minor comments re: how to handle Struct fields |
nudge @TylerLubeck |
Hey sorry, I've been away from Kafka for a while now. I think I've got this set up to not require double storing all the attributes. One of the reasons I had gone in that direction before was to avoid accidental use of other, non-schema items in |
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, looks great!
There are a handful of places that call out for converting response tuples to more pythonic objects. This is a stab at it - I'm happy to make changes if this isn't what you were thinking
This change is