feat!: use sets for the state components

[tonyandrewmeyer]

The primary change is that instead of lists for the various State components (containers, relations, etc), frozensets are used.

• This increases the immutability of State objects - ie. it's no longer possible to do something like state.containers.append(Container(...)), which people shouldn't have done previously, but would work.
• This prevents people from using magic numbers to refer to components of the state that are inherently unordered - ie. no more assert state_out.relations[0]....

To provide access to the components, and to avoid people needing to write many rel = any(r for r in state_out.relations if r.id == id) type statements, there are several new methods on State:

• get_container to get a container by name (this previously existed, and has been simplified to match the others, so it can no longer take a Container object)
• get_relation to get a relation by ID
• get_stored_state to get a stored state by name and owner path
• get_storage to get a storage by name and index
• get_relation to get a relation by ID
• get_network to get a network by binding name

get_resource and get_opened_port are not added, because the charm cannot add or change resources, and ports don't have a unique identifier (it's more likely that tests want to check whether a Port is contained in the set).

There are two straight renames that change from singular to plural, since the state holds 0-n:

• stored_state becomes stored_states (I can see that it's all "stored state", but there are multiple StoredState objects, so the plural seems more correct)
• storage becomes storages (charmcraft.yaml does use "storage", but I feel it's better to be consistent within Scenario than copy a perhaps-poor decision in the config)

Several state components gain a custom __hash__ method, so that they can be put into sets/frozensets. This also has the side-effect of removing the need to have the consistency checker check for duplicates, because a set/frozenset can't have a duplicate. It does mean that you can pass a set that has duplicates and not get a warning (Python just discards one item), but a linter should detect that issue, as with regular set use.

The PR also introduces a Resource class to hold resources, so that they are consistent with all of the other components of the state.

[PietroPasotti]

very nice!
wondering if the get_x methods should (also) take instances as arguments

[tonyandrewmeyer]

very nice! wondering if the get_x methods should (also) take instances as arguments

Yeah, it seems wrong that get_container does (because it already did in 6) and the others do not. It does seem like consistency would be best, and there doesn't seem to be a reason to remove it from get_container, so adding to the others would probably be cleanest.

[benhoyt]

API changes look consistent with what we discussed -- good work! (I haven't done a line-by-line review -- maybe others can.)

One question I have: is it weird that users will be putting regular sets with {...} syntax in, but get frozensets out? Should we just use regular old sets everywhere, even though they're "more mutable"?

[tonyandrewmeyer]

One question I have: is it weird that users will be putting regular sets with {...} syntax in, but get frozensets out? Should we just use regular old sets everywhere, even though they're "more mutable"?

I did wonder about this. I strongly believe that frozensets are the most appropriate data type - but it's super inconvenient to use them for the input since there's no literal syntax.

It is a bit magic / inconsistent - but I think less so than if you were putting in an entirely different type. It's just "freezing" the content you put in, so I felt like that was ok.

I do actually like that you can provide any iterable (of hashable objects) and you'll get the expected state object - so you can do State(containers=(cont1, cont2)) or State(containers={cont1, cont2}) and you'll end up with the same .containers == frozenset({cont1, cont2}). This feels more Pythonic to me - duck typing rather than strict typing - even though it's not the typical dataclass result.

I think it's also beneficial to increase the immutability of State, since we're expecting charmers to treat it as completely immutable. If it's possible to state.containers.add(cont3) then people will, leading to unexpected behaviour.

[benhoyt]

Yep, I agree with that reasoning -- duck/loose typing accepted as input, strict/best type used as output.

[tonyandrewmeyer]

The 'get' signatures have ended up more mixed than they were previously.

Before:

• get_container(self, container: Union[str, Container]) -> Container
• get_secret(self, *, id: Optional[str] = None, label: Optional[str] = None) -> Secret
• get_stored_state(self, name: str, owner_path: Optional[str] = None) -> StoredState
• get_storage(self, name: str, index: Optional[int] = 0) -> Storage
• get_relation(self, id: int) -> "AnyRelation"

Now:

• get_container(self, container: Union[str, Container], /) -> Container
• get_secret(self, *, id: Optional[str] = None, label: Optional[str] = None, secret: Optional[Secret] = None) -> Secret
• get_stored_state(self, stored_state: Union[str, StoredState], /, *, owner_path: Optional[str] = None) -> StoredState
• get_storage(self, storage: Union[str, Storage], /, *, index: Optional[int] = 0) -> Storage
• get_relation(self, relation: Union[int, Relation]) -> "AnyRelation"

@benhoyt @PietroPasotti would you still go with this? Having a bunch of "which set of arguments were passed and do they all agree with each other" code is a smell imo. I think it works ok for Container and Relation but less so for StoredState and Storage (and Secret is odd either way, because of the need to get by label).

[PietroPasotti]

Is it an idea to use singledispatch? That would clean up the signatures imho, at the cost of more lines of code.

[tonyandrewmeyer]

TIL about functools.simpledispatch, thanks! I guess it's basically sugar for a bunch of if isinstance(arg1, x) calls, and presumably makes the type signatures nicely too. I don't mind the extra verbosity, and it probably would indeed simplify the cases where you're passing the wrong combination of args (I assume apart from the static checking, it actually fails at runtime if you do that too). However, it doesn't feel particularly Pythonic (despite being in the stdlib). It also still leaves the signatures fairly complex.

@benhoyt and I talked this over and decided to start with the simpler key-only signatures (simplifying get_container). We can always expand on them later if people find that passing in the object really would be a significant improvement. For container, for example, it's only the difference between get_container(container) and get_container(container.name). More extensive remap type functionality might make it unnecessary, also.