Skip to content
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

Add a "modifiers" hook to allow extension authors to mutate/redact event data #12

Merged
merged 11 commits into from
Aug 22, 2022

Conversation

Zsailer
Copy link
Member

@Zsailer Zsailer commented Aug 16, 2022

Addresses #11

  • Modifiers are added before the event is validated and emitted. That means, the mutated event are still required to follow the event schema.
  • Modifiers are applied to all incoming events. Authors of modifiers can special case events inside their functions. This is slightly different than the listeners API in Add Listeners API #10, which attaches listeners to specific events.
  • This enforces a strict function/method signature (types included) for all modifiers added to the eventlogger.

@Zsailer Zsailer added the enhancement New feature or request label Aug 16, 2022
@Zsailer
Copy link
Member Author

Zsailer commented Aug 16, 2022

This is ready for final review and should likely be merged before #10.

@Zsailer Zsailer mentioned this pull request Aug 16, 2022
2 tasks
Copy link
Contributor

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@Zsailer
Copy link
Member Author

Zsailer commented Aug 17, 2022

Thanks @blink1073!

I just added a new commit: 3c5689d. Do you mind taking another look? 😃

After working with this API more and thinking about previous chats we've had, I think making all of the public method/function APIs take dataclasses as arguments is a better approach. This gives us, the jupyter_events developers, more flexibility to change these APIs without causing backwards incompatibility.

@blink1073
Copy link
Contributor

I don't think we need a separate class for ModifierData, it makes sense to me to return a new Event if you wish to modify any fields.

@Zsailer
Copy link
Member Author

Zsailer commented Aug 17, 2022

My reasoning for separating these dataclasses was thinking ahead about possible changes to the emit(...) method signature or the modifier signature, modifier(...), in the future.

Today, Event and ModifierData look equivalent, so .emit(data: Event) and def my_modifier(data: Event) could work. But what if we wanted to add arguments to my_modifier that don't belong in the `.emit(...) method? I was thinking we might want to decouple these input types so that these APIs could evolve independently from each other.

That said, I agree it would be much nicer to just have the Event dataclass. We could make any new attributes in the Event object optional in the future to allow for backwards compability.

@Zsailer
Copy link
Member Author

Zsailer commented Aug 17, 2022

Ok, I've consolidated these dataclasses 👍. It makes the API really nice and clean. Thanks @blink1073!

Copy link
Contributor

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Contributor

@yuvipanda yuvipanda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hacking with @Zsailer in person!!! More comments left in person :D

jupyter_events/logger.py Outdated Show resolved Hide resolved
jupyter_events/logger.py Outdated Show resolved Hide resolved
@@ -54,6 +65,8 @@ class EventLogger(Configurable):
""",
)

modifiers = Dict({}, help="A mapping of schemas to their list of modifiers.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this list be private? I think it makes sense for it to be private and to have a remove_modifier function.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Great idea. Fixed!

Copy link
Contributor

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants