Skip to content
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

Closed
wants to merge 2 commits into from

Conversation

rcfox
Copy link
Contributor

@rcfox rcfox commented Dec 16, 2015

(If the value is a Structure, then replace the contained one.)

Currently, setting to a SubstructureField directly will replace the field with whatever you set, which can break the parent structure if what you set isn't a BaseField type.

class PascalString16(Structure):
    length = LengthField(UBInt16())
    value = Payload(length)

 class Name(Structure):
    first = SubstructureField(PascalString16)
    last = SubstructureField(PascalString16)

name = Name()
name.first = 'bob' # Now name.first is a str instead of a PascalString16!

With this change, name.first = 'bob' will call setval on the PascalString16 structure under name.first. This might not be a meaningful operation for many Structure types, but at least it will now raise an exception if setval hasn't been defined.

…e. If the value is a Structure, then replace the contained one.
@@ -8,6 +8,7 @@
import six
from suitcase.exceptions import SuitcaseChecksumException, SuitcaseProgrammingError, \
SuitcaseParseError, SuitcaseException, SuitcasePackStructException
import suitcase
Copy link
Contributor

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:

from suitcase.structure import Structure

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@posborne
Copy link
Contributor

Currently, I'm leaning away from overloading the assignment operator in this way. See comments. For PRs to be merged, tests must be passing as well.

@rcfox
Copy link
Contributor Author

rcfox commented Jan 13, 2016

I decided to just implement this locally within my own project:

class PassthroughSubstructureField(SubstructureField):
    ''' Pass assignments of non-Structure values through to the the contained Structure, rather than overwriting it. '''
    def setval(self, value):
        if value is None or isinstance(value, Structure):
            super(PassthroughSubstructureField, self).setval(self, value)
        else:
            self._value.setval(value)

@rcfox rcfox closed this Jan 13, 2016
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.

None yet

3 participants