Decode dynamic structures in a sensible manner

[florczakraf]

Background

I'm often struggling with decoding structures that consist of dynamic arrays. I'm unable to carve the exact payload to provide to the decode method because there are no extra markers in the blob I'm working on. When there are some bytes in the payload after decoding, prophy raises enigmatic ProphyError (but it's message is meaningful). I'd like to simplify the logic that a user of the library has to write in such case. For example:

try:
    struct.decode(payload, endianness='<')
except ProphyError as e:
    if 'not all bytes of' not in str(e):
        raise

Simple error change

We could just subclass the ProphyError with something more explicit to use in this particular case. For example:

class ProphyRemainingBytesError(ProphyError):
    pass

then the user's code could be just:

try:
    struct.decode(payload, endianness='<')
except ProphyRemainingBytesError:
    pass

Potential bug

I think there's a bug related to the behavior I'm observing. To fix it, the library will require slight change in the public interface.

For some reason, encode method can receive terminal argument but it's unused. On the other hand, decode lacks such argument but it fills the internal's _decode_impl terminal with True all the time. If we could add terminal=True to decode, then the user could explicitly state that the structure she is trying to decode is not the final one in the payload and therefore the mentioned error would not be raised at all. The above use case could be compressed to just:

struct.decode(payload, endianness='<', terminal=False)

Summary

I think that we could implement the new error independently from the second part. As for the interface change, adding terminal=True to decode is just an extension of the interface so it shouldn't be a big problem to introduce it at any time. It turns out that the encode case is being covered by @kamichal's #16 in the meantime.

[aurzenligl]

class ProphyRemainingBytesError(ProphyError):

That's a good idea.

Inheritance vs composition: I have only one suggestion - inheritance allows to differentiate information across only one dimension (e.g. things get complicated if we'd like to throw ErrorX and ErrorY, which both occur in different hierarchies stemming from ProphyError. Then we'd need to do a diamond-like inheritance to throw a single error derived from classes from both hierarchies). Using methods or attributes - on the other hand - allows to differentiate information in exception across many attributes independently. Since exception of given class should implement all such attributes/methods/properties, it's probably better to subclass ProphyError by a single error class related to whole family of problems, e.g. prophy.DecodeError

I'd definitely prefer to shy away from public 3-or-more-word identifiers, since:

1. ProphyRemainingBytesError seems like a too detailed error category, difficult to associate with decode/encode (relates to one of them or both?, relates to anything else, e.g. printing or copying?).
2. prophy public namespace is sparsely populated with names, collision is very unlikely,
3. user will anyway have names neatly separated in "prophy." namespace - they won't collide with his names (and I'd strongly prefer prophy.DecodeError in user code than ProphyDecodeError (I know where prior type is defined exactly - in third party code - but latter can be a user's wrapper: dot conveys additional, useful meaning)),
4. the longer the name, the harder it is to remember, spell and distinguish from other long names.

I'd propose a DecodeError which could expose in public attributes/properties specific information about the decode function error - hopefully allowing to service a whole family of different decoding errors.

For some reason, encode method can receive terminal argument but it's unused.

I could trace adding terminal optional argument to encode to commit 3ef1dc1 (pre-0.2.3), which was needed at that time for padding. Padding logic has been put in different place since then - so now endianness argument is a leftover - I wouldn't hesitate to remove it from API in next minor change.

struct.decode(payload, endianness='<', terminal=False)

That can be added, sure, but let's think what we'd like to achieve this way and whether this is the most readable/maintainable way to get there. terminal decides only about execution of this if block:

        if terminal and pos < len(data):
            raise ProphyError("not all bytes of {} read".format(self.__class__.__name__))

Next line is an unconditional return from function, so the meaning is:

• if terminal, raise when full input buffer was not decoded,
• if not terminal, never raise.

I suppose optional argument raises (True by default) would be way more obvious for readers. A separate classmethod or free function decodes returning True/False or tuple, or an object with named attributes could be yet another solution (then we'd have separated two use cases - 1) decoding just to decode and fire exception in case of error, 2) only checking whether buffer is decodable).

[aurzenligl]

Summing up: we seem to have two separate problems discussed in this issue:

1. subclassing ProphyError for decode-related problems,
2. decide on prophy user side whether decode call raises.

Which of these problems would you like to solve in related PR - and how?