Correct type handling for bitwise variable access#663
Conversation
Contrary to the existing type hints, the methods can handle strings for lookup as well as an arbitrary collection of integer bit offsets. Correct the hints to reflect that and check at runtime to only lookup from strings. Correct return type for encode_bits(), as it works only for integers. Document both functions to explain the intention and caveats to consider, especially the lookup KeyError exception. Extend test to cover an iterable and a failed lookup.
Test encoding and decoding with non-contiguous bit offsets.
Any bit for which the replacement pattern contained a 1 bit in one of the "holes", would be set despite not being part of the desired mask.
This mechanism works only for integers, so add appropriate assertions. Document all expected types for indexing the Bits.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
This should properly fix some issues that #651 only silences. |
| class Bits(Mapping): | ||
|
|
||
| def __init__(self, variable: Variable): | ||
| assert variable.od.data_type in objectdictionary.datatypes.INTEGER_TYPES |
There was a problem hiding this comment.
As I learned like five minutes ago, it seems that -O or -OO flags (mean running python in optimized mode) will disable assertions and thus it is always better to use if / raise. Personally I like the inline assertions a lot and use them quite often. Also libs like pydantic use them in business logic. AI complains you should only use assertions in tests and otherwise raise an appropriate exception (probably TypeError).
There was a problem hiding this comment.
Thanks for the pointer, I didn't know either how easy it is to skip them.
The real question here is what API guarantees we want to give. Bitwise access for a non-integer object is a programming error. Catching that early and reporting in a meaningful way is important during development (of the application I mean, not the library). But once the application straightens out its use of objects and the corresponding types, I see nothing wrong with skipping this check for performance, if optimization is requested by the user. There will be an exception at some point, but disabling assertions on unverified code is basically asking for less useful error diagnostics.
Contrary to the existing type hints, the
decode_bits()/encode_bits()methods can handle strings for lookup as well as an arbitrary collection of integer bit offsets. Correct the hints to reflect that and check at runtime to only lookup from strings.Correct return type for encode_bits(), as it works only for integers.
Document both functions to explain the intention and caveats to consider, especially the lookup KeyError exception.
Extend test to cover an iterable and a failed lookup.