-
Notifications
You must be signed in to change notification settings - Fork 13
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
Fix conditional substructures #27
Fix conditional substructures #27
Conversation
TODO: Write unit tests to completely verify fix. Fixes digidotcom#25
def is_substructure(field): | ||
return isinstance(field, SubstructureField) or \ | ||
isinstance(field, ConditionalField) and \ | ||
isinstance(field.field, SubstructureField) |
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.
Wondering if it might be better to do this polymorphically.
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 can change this. The main reason for changing is so that user's can add fields that behave similar to ConditionalField
(wrapping something like a SubstructureField) without having to modify this code (Open/Closed Principle)
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.
The other thing is that there are other fields that could wrap SubstructureField
such as LengthField
that should have similar logic. I'm fixing this with the change to use polymorphism (i.e. field.is_substructure()
).
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.
Yeah, this check is kind of a hack and I only really tested for the Substructure-in-Conditional case because that was the first obvious problem. If you've got a way to do this generally, by all means.
@mikewadsten, take a look at 2a8a86c. If that looks good, I'll pull it into master (it builds on your branch). |
@posborne Looks good to me. 👍 |
This fix mostly consists of making
Packer
recognizeSubstructureField
inside a conditional field, and making sureConditionalField.getval
returns None when the condition is not met. (Without the latter, it would always return the uninitialized substructure when accessed through the parent structure, e.g.message.f2
)