Rewrite t.Struct to use type annotations

[puddly]

I've rewritten t.Struct to behave more like a normal Python class. Before:

class TestStruct(t.Struct):
    _fields = [("foo", t.uint8_t)]

s = TestStruct(foo=1)
print(s.foo)  # it's None :(
s.foo = 2
print(s.serialize())  # doesn't work :(

After:

class TestStruct(t.Struct):
    foo: t.uint8_t

s = TestStruct(foo=1)
print(s.foo)  # it's 1
s.foo = 2
print(s.serialize())  # still works

I had to change a few types around to make things work:

• The OTA FileImage was a subclass of OTAImageHeader. It isn't anymore and now stores the image header under the header attribute.
• Date._year has been renamed to Date.years_since_1900.

[coveralls]

Coverage remained the same at 100.0% when pulling 2a0de4f on puddly:feature/struct-rewrite into 29e3625 on zigpy:dev.

[Adminiuga]

this is great. Love it so much.
Are you running this in your production?

[puddly]

I'm planning on doing that later today. It doesn't look like t.Struct is used by any external library so it should just work.

The only real source of bugs that I can see is stuff like t.Bytes(13).serialize(), where the field type doesn't break when serializing the wrong type. Explicit type checking isn't very Pythonic but it's relatively simple to implement now that all of the major type bugs have been fixed in zigpy (at least the ones I've found in production).

[Adminiuga]

Switched to your branch in my production. So far looks good. it better stay that way :)

[Adminiuga]

But this does look good. Going to steal borrow it for bellows

[puddly]

I'm still exploring a few ways to clean up and improve this code to make it more reusable:

1. Maybe using inspect's signature functions or dynamically generating __init__ like attr?
2. Making structs immutable would help with the type problems a bit, since they all can easily be checked during object creation. Adding a .replace(thing=0x1234) method to modify fields should make the transition very simple.
3. Expanding the Field data class to also store an optional bool and a description string should make the Struct class useful for cleaning up the ZDO layer. It'd also replace the Optional subclass hack.

[Adminiuga]

Should we explore approach attrs package is taking?
And yeah, it would be nice to expand Field data class to mark Optional attributes and maybe adding a conditional e.g. if self.status != Success then skip serialization, deserialization

[puddly]

How about something like this?

import typing


class OptionalStruct(t.Struct):
    foo: int
    bar: typing.Optional[int]
    #bar: typing.Optional[int] = t.StructField()  # same thing as above

# Both work
OptionalStruct(foo=1, bar=1).serialize()
OptionalStruct(foo=1).serialize()


class ComplexOptionalStruct(t.Struct):
    foo: int
    status: t.Status
    bar: typing.Optional[int] = t.StructField(depends_on=lambda s: s.status == t.Status.Success)

# These work
ComplexOptionalStruct(foo=1, status=t.Status.Success, bar=2)
ComplexOptionalStruct(foo=1, status=t.Status.Failure)

# This doesn't
ComplexOptionalStruct(foo=1, status=t.Status.Success)


# This breaks a lot of unit tests but I don't see a reason to allow structs to be
# created with non-keyword arguments. So this:
nd1 = t.NodeDescriptor(0, 1, 2, 0x0303, 0x04, 0x0505, 0x0606, 0x0707, 0x08)

# Would have to be created like this:
nd2 = t.NodeDescriptor(
    byte1=0,
    byte2=1,
    mac_capability_flags=2,
    manufacturer_code=0x0303,
    maximum_buffer_size=0x04,
    maximum_incoming_transfer_size=0x0505,
    server_mask=0x0606,
    maximum_outgoing_transfer_size=0x0707,
    descriptor_capability_field=0x08
)

# And this would break because structs are immutable
nd2.server_mask = 0xFFFF

# But you could do something like this instead
nd3 = nd2.replace(server_mask=0xFFFF)

[Adminiuga]

This breaks a lot of unit tests but I don't see a reason to allow structs to be created with non-keyword arguments. So this:

I don't have a strong opinion on this one. In fact I do agree that using keyword arguments would be much more descriptive. Just ATM I think there are a few places where NodeDesc might be created with args. Would need to check zhaquirks and HA tests (iirc ZHA tests use deserialize instead, so shouldn't be a problem) but we can update those if we have to. I'd rather go toward more readability.

Otherwise this looks great

[puddly]

I renamed depends_on to skip_if and flipped the logic but otherwise the new struct class is finished, tested, and works as it should.

As far as compatibility, it looks like there are at least two places in quirks where the NodeDescriptor is created with positional arguments. The public interface for t.Struct should be the same (well, other than the immutability) so I think it might be better to allow positional argument construction for now and then just disable it again once quirks has been updated.

One source of friction so far when fixing the unit tests is that a lot of objects are created "empty" and then a few attributes are set, sometimes as sentinel values. This conflicts with the construction-time checking of arguments (if a struct exists, it's valid and can be serialized) so a bunch of unit tests will have to be adjusted to work with complete, valid objects. Alternatively the creation-time constraints for structs can be relaxed but I feel like this conflicts with their purpose.

[Adminiuga]

how many tests would need updates?
Maybe should split this int two PRs? I'm running the yesterday version and haven't noticed any ill effects

[Adminiuga]

in other words I'm happy to merge the yesterday version without last commit as is and build on top of that. Would rather have a few smaller PRs.

[puddly]

Yeah, that sounds reasonable. I'll split off the more complex version into a separate branch and just fix up the drop-in replacement.

[puddly]

I was able to slightly tweak the more feature-rich struct class to also be a drop-in replacement for the old one while passing all of the unit tests, including the new struct ones. It should now be possible to incrementally simplify struct subclasses that have optional fields that also depend on the presence/value of other fields (e.g OTAHeader) without having to make a ton of changes all at once.

Getting all unit tests to pass with the strict t.Struct type is doable but it requires changing a bunch of classes (though a few were significantly simplified). I don't have enough experience with ZCL's foundation to know how and why everything is implemented the way it is but here are the changes (puddly/zigpy@puddly:feature/struct-rewrite...puddly:feature/struct-rewrite-v2) required to get the non-skipped pytest tests to pass. It's about half done.

I'm debating whether or not to rename the skip_if back to depends_on. The former seems to be harder to parse on first glance.

[Adminiuga]

I'm debating whether or not to rename the skip_if back to depends_on

Yeah, this is a good one. I was thinking if validate or validator would sound better, but still not quite the same.

[Adminiuga]

Let me know when it is ready for review. I glanced through ZCL foundation changes and it looks fine.

[puddly]

I missed a couple of instances of _fields in the ZCL General cluster definitions but those are now fixed (and explicitly checked for). Otherwise, it's good to go.