Skip to content

Conversation

zaxrok
Copy link

@zaxrok zaxrok commented Feb 19, 2018

my branch is rebased on your the recent master
I hope to pass on travis
Thanks!

members = {}
for s, stype in six.iteritems(members_types):
schema_instance = RosSchema()
for s, stype in members_types.iteritems():
Copy link
Member

Choose a reason for hiding this comment

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

iteritems() is not py2/py3 compatible. please use methods that work in both.

members_types = _get_rosmsg_fields_as_dict(ros_msg_class)
members = {}
for s, stype in six.iteritems(members_types):
schema_instance = RosSchema()
Copy link
Member

Choose a reason for hiding this comment

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

So you mean that instead of creating a child class (CustomMsgSchema) of RosSchema, and taking and instance of that class,
it is better to instantiate a RosSchema and usedirectly that instance ?
How about the second time we need that Schema, we have to create it again it seems ?

Copy link
Author

Choose a reason for hiding this comment

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

I tried to solve your source as much as possible without modifying it if possible. So I did not try various methods. Sorry and thank you of your efforts.

ros_schema_inst = RosNested(create(stype)) # we need to nest the next (Ros)Schema

members.setdefault(s, ros_schema_inst)
schema_instance.declared_fields[s] = ros_schema_inst
Copy link
Member

Choose a reason for hiding this comment

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

This seems very hacky to me.
If marshmallow is updated and decides to change the internals of RosSchema, nothing will work and it will be hard to find this place again. Why not initializing the instance after we have gathered all members ?

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.

3 participants