Attention is currently required from: Hoernchen.
Patch set 1:Code-Review +1
5 comments:
Patchset:
I have looked through your patch and I think it is plausible, however it introduces a state that other TLV classes don't have. Maybe this needs some explanation.
Commit Message:
Patch Set #1, Line 35: instead of setting the flag, currently tag=0x8123 -> 0x81a3.
I would split this in a separate patch (only if you think this would speed things up, as I think this part would be ready at least.)
File src/osmocom/tlv.py:
Patch Set #1, Line 658: # True/False: as received
Maybe it makes sense to explain this property in greater detail? This is an additional state an API user would expect right away. I understand this as a state that flips to the state of the last parsed CR bit when from_tlv() has been used. This basically means that when I have used from_tlv() once, the API user cannot rely on the CR bit set as originally defined.
This is probably a corner case as the IE objects are usually short lived and probably not wildly mixed. Decode, change something, Re-Encode is probably the most exciting usecase to expect but I still think we should have an explanatory comment here.
File tests/test_tlv.py:
I would add a comment that explains that the first vector has the CR bit set, but the second has not. And both must come out the same when they are decoded and re-encoded.
Patch Set #1, Line 86: ie.from_tlv(encoded)
maybe add an assert that checks that ie.comprension has changed state as expected?
To view, visit change 43171. To unsubscribe, or for help writing mail filters, visit settings.