Closed tonyandrewmeyer closed 3 months ago
one note: not sure we can hide Event
as that is necessary for testing custom events.
edit: scratch that, I see now you were proactively removing support for that use case.
@PietroPasotti and @benhoyt are you both ok with this proposal, e.g. the usage as outlined in the PR description? If so, then I'll go ahead and polish this PR up so that it's in a properly reviewable, but it would nice to have a tentative yes for the approach first.
In principle I'm ok with the approach, although I'm not sure I like it more than the original. Two doubts I have left, ux-wise, are:
1- how often will the user try to do:
ctx.run(ctx.on.baz_relation_departed, state)
only to find out that the ordinal arguments are required?
Do we add two separate event types to make at least the type checkers aware that you can't just pass whatever ctx.on.baz_relation_departed
is to ctx.run()
without calling it first?
It would be easier if all events were callable (and HAD TO be called when passing them to .run()
).
2- Why are some on.x
callable and some not?
What happens if you try to ctx.run(ctx.on.install(), state)
instead of ctx.run(ctx.on.install, state)
?
Again it would be easier if all events were callable (and HAD TO be called when passing them to .run()
).
# Simple events: no args
ctx.run(ctx.on.install(), state)
# Secret events:
ctx.run(ctx.on.secret_changed(secret=secret), state)
ctx.run(ctx.on.secret_expired(secret=secret, revision=revision), state)
# Storage events:
# storage = Storage("foo", ...)
# state = State(storages={storage})
ctx.run(ctx.on.storage_attached(storage=storage, state)
# Action events:
ctx.run_action(ctx.on.action(action=action), state)
# Container events:
ctx.run(ctx.on.pebble_ready(bar_container), state)
# Relation events:
ctx.run(ctx.on.relation_created(relation=baz), state)
ctx.run(ctx.on.relation_changed(relation=baz), state)
ctx.run(ctx.on.relation_changed(relation=baz, unit=unit_ordinal), state)
ctx.run(ctx.on.relation_departed(relation=baz, unit=unit_ordinal, departing_unit=unit_ordinal), state)
it's also way better for autocompletion, since ctx.on.foo_relation_changed
would be trouble to dynamically populate.
Yeah, I kind of like the consistency and type-simplicity of @PietroPasotti's "counterproposal". @tonyandrewmeyer let's discuss today in our 1:1 and see if we can come to agreement here.
Decision is to go with Pietro’s counterproposal. Keep ctx.on because it’s a nice signal that it’s “an event type thing” and nice namespace/autocomplete. Also, Zen of Python says “If the implementation is easy to explain, it may be a good idea.”
This PR adds the ability to specify the event to run using
ctx.on
, for example:This also removes the ability to run custom events explicitly (they are still run implicitly when a Juju event triggers them). As discussed previously, if there's demand for this in the future then we'll consider adding it back, and decide on an API at that time.
A few fixes slipped in:
tox -e lint-tests
was linting both tests and the main code, which I assume it was not meant to doevent.unit
in a -relation-broken and a -relation-created event should always be Nonesecret-expired
andsecret-removed
(at least I couldn't figure out how to get it to work), so that's also done here