-
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
Pass setval calls through SubstructureField to the contained Structure #31
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ | |
import six | ||
from suitcase.exceptions import SuitcaseChecksumException, SuitcaseProgrammingError, \ | ||
SuitcaseParseError, SuitcaseException, SuitcasePackStructException | ||
import suitcase | ||
from six import BytesIO, StringIO | ||
|
||
|
||
|
@@ -831,6 +832,12 @@ def unpack(self, data, **kwargs): | |
self._value = self.substructure() | ||
return self._value.unpack(data, **kwargs) | ||
|
||
def setval(self, value): | ||
if isinstance(value, suitcase.structure.Structure): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps I am thinking slowly today. Is there some instance where this would not be the case? I think this change makes sense, but I'm wondering if it might make more sense to do something like this instead: def setval(self, value):
if not isinstance(value, self.substructure):
raise TypeError("SubstructureField expected %r instance, got %r" % (self.substructure, type(value)))
else:
self._value = value There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I was thinking slowly. I see what you are trying to do here. I've been thinking for awhile about having a way to instantiate new Structures without having to assign to each field individually. I think that could help with this problem as well. This would look something like this: # Arguments for creating a new structure with fields initialized
Structure(*positional_field_values, **field_values_by_key)
# Assign to members of existing structure in memory
structure.set(*positional_field_values, **field_values_by_key) The Does this sound like an acceptable approach? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So what I was thinking was:
Anyway, maybe it's a little too magical, but I definitely think an exception should be thrown for overwriting a Structure with a non-Structure. |
||
BaseField.setval(self, value) | ||
else: | ||
self._value.setval(value) | ||
|
||
|
||
class FieldArray(BaseField): | ||
"""Field which contains a list of some other field. | ||
|
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.
Stylistically, I think I would prefer:
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 agree that it would look nicer, but it introduces a circular dependency between fields.py and structure.py.
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.
That could be resolved by adding
from suitcase.structure import Structure
to the setval method.