Open PietroPasotti opened 3 months ago
Current code:
relation = scenario.Relation("foo", local_app_data={"foo": "bar"}) state = scenario.State(relations=[relation]) state_out = context.run(...) relation_out = [r for r in state_out.relations if r.relation_id == relation.relation_id][0]
For what it's worth, as long as you have the original relation/relation ID, then you can just do this:
relation_out = state_out.get_relation(r.id)
I've seen tests where the state comes as a fixture and so the original relation isn't conveniently available, but remap
would have the same issues in that case, I think.
with
remap
:relation = scenario.Relation("foo", local_app_data={"foo": "bar"}) state = scenario.State(relations=[relation]) state_out = context.run(...) relation_out = state_out.remap(relation)
It seems like the main advantage over get_relation
is that you can just always use remap
rather than having to pick between get_relation
, get_container
, and so on. It's also potentially more convenient to pass the object than its identifier.
Overall, I'm -0 on remap
.
State.patch
"modify just this component"
Often you want to dynamically create 'variations' of a State:
relation = Relation('foo', local_app_data=A) state = State(relations=[relation, ...], ...) relation_but_different = relation.replace(local_app_data=B) # copy over all relations except the one we're changing, inject that one separately state_but_different = state.replace(relations=[r for r in state.relations if r.name != "foo"] + [relation_but_different])
This needs to use dataclasses.replace
now, although it's roughly the same other than needing the import:
relation_but_different = dataclasses.replace(relation, local_app_data=B)
state_but_different = dataclasses.replace(state, relations=[r for r in state.relations if r.name != "foo"] + [relation_but_different])
With
State.patch
:
I'm +1 on patch.
State.insert
Minor nit: probably this should be State.add
since they're sets not lists now?
+1 on insert
(or add
).
State.without
For what it's worth, I think State.remove
would be more consistent.
"Remove just this object"
counterpart to
insert
but removing stuff. withwithout
:relation = Relation('foo', local_app_data=A) state = State(relations=[relation, ...], ...) state_but_different = state.without(relation) assert relation not in state_but_different
I'm pretty certain this could be:
relation = Relation('foo', local_app_data=A)
state = State(relations={relation, ...}, ...)
assert relation not in state.relations
That seems simpler to me.
# but also: state_but_different = state.without(relation). without(secret).without(storage).without(storedstate)
And similar here, I think I would rather do:
assert relation not in state.relations
assert secret not in state.secrets
assert storage not in state.storages
assert storedstate not in state.stored_states
# or
assert relation not in state.relations and secret not in state.secrets and storage not in state.storages and storedstate not in state.stored_states
So -1 on without
unless there's something I'm missing here.
Edit: I guess the better use-case is not asserting whether something is still in the state (e.g. did a secret get removed) but setting up a state for another event. I suppose the symmetry with adding/inserting is nice in that case. So I guess I'm -0 on without
.
I'm not a fan of these. I'd much rather have a little bit of boilerplate that any Pythonista can understand, rather than starting to create our own little domain-specific language. Specifics:
remap
confusing. And as Tony pointed out, that's why we added that get_container(r.id)
and similar methods, which also have simpler type signatures.dataclasses.replace
or explicitly recreate the object. If anything, we could add State.replace
to make it a little shorter: state.replace(foo=bar)
rather than dataclasses.replace(state, foo=bar)
.dataclasses.replace
with +
or |
on the field. Tiny bit longer, but more obvious and explicit.dataclasses.replace
again and filter.I could maybe see myself coming around to State.replace
as a shorthand for dataclasses.replace
, but I'm not a fan of adding a bunch of methods.
However, to make a more informed decision, I'd rather see this in the context of actual, real-world tests, to see what it's actually saving, and whether there are other/better ways to achieve the same thing.
This PR adds to
scenario.State
a few utility methods for manipulating state objects.State.remap
"get me the corresponding object"
use case:
You create an object (relation, storedstate, secret, network, storage...), put it in State along with potentially many others You run scenario You want to retrieve the 'same' object and see how it was modified by the charm However scenario's objects are immutable, so you can't do that easily without manually filtering all objects...
Current code:
with
remap
:State.patch
"modify just this component"
Often you want to dynamically create 'variations' of a State:
With
State.patch
:(this could also be used to do delta-based comparisons):
State.insert
In order to dynamically create modified copies of a state, right now you're limited to using the
replace
api. Suppose you want to add a relation:with
insert
:State.without
"Remove just this object"
counterpart to
insert
but removing stuff. withwithout
: