canonical / ops-scenario

State-transition testing SDK for Operator Framework Juju charms.
Apache License 2.0
10 stars 7 forks source link

fix: state components take an Iterable rather than a frozenset #160

Closed tonyandrewmeyer closed 1 month ago

tonyandrewmeyer commented 1 month ago

This changes the signature for State but the actual implementation stays the same:

The advantage of this change is that without it, if you do:

State(containers={container})

Then type checkers will complain with something like Argument of type "set[str]" cannot be assigned to parameter "x" of type "frozenset[str]" in function "__init__" "set[str]" is incompatible with "frozenset[str]" (reportArgumentType).

The main disadvantage is that the auto-generated reference docs can't tell that the __post_init__ conversion happens, so can only say that State.containers is an Iterable, which hides the "freezing" - callers might reasonably expect that it will be set to whatever type they pass in. However, I think we can likely solve this with tweaks to the documentation.

A small drive-by fix of the API docs also.

Fixes #159.

PietroPasotti commented 1 month ago

yeh, and in general when working with dataclasses (or dataclass-like things) there's a very reasonable expectation that

type(Foo(x=X()).x) is X

which we're violating

but I get the point that frozenset is a verbose piece of specific syntax

tonyandrewmeyer commented 1 month ago

yeh, and in general when working with dataclasses (or dataclass-like things) there's a very reasonable expectation that

type(Foo(x=X()).x) is X

which we're violating

Yes, :100:.

but I get the point that frozenset is a verbose piece of specific syntax

Yeah, if Python just had some literal for frozensets then I would definitely prefer the type to match. Practicality beats purity.