-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: support 'ctx.on.event_name()' for specifying events #126
Conversation
one note: not sure we can hide 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. 1- how often will the user try to do:
only to find out that the ordinal arguments are required? It would be easier if all events were callable (and HAD TO be called when passing them to 2- Why are some Counterproposal
it's also way better for autocompletion, since |
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.” |
The failing test are (a) ones that need to be rewritten for the new system, (b) ones to do with custom events.
These are not as valuable without support for emitting custom events directly, but it seems worthwhile to leave them in so that if we add custom event emitting back in the future they're here to build off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great! looking forward to trying it out :)
@@ -245,7 +247,12 @@ def _get_event_env(self, state: "State", event: "Event", charm_root: Path): | |||
remote_unit = f"{remote_app_name}/{remote_unit_id}" | |||
env["JUJU_REMOTE_UNIT"] = remote_unit | |||
if event.name.endswith("_relation_departed"): | |||
env["JUJU_DEPARTING_UNIT"] = remote_unit | |||
if event.relation_departed_unit_id: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huh, good catch. this was wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't done a super-close look, but I've scanned over the whole thing, and this looks good to me. I quite like how it looks, and the implementation is significantly simpler.
The failing test are (a) ones that need to be rewritten for the new system, (b) ones to do with custom events.
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