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

Enable adding listeners to event before the event is registered #97

Merged
merged 3 commits into from
Mar 18, 2024

Conversation

Zsailer
Copy link
Member

@Zsailer Zsailer commented Mar 14, 2024

Currently, if listeners are added before the event they are meant to watch is added, the listener gets removed when the event is registered. We should keep them around.

This showed up in work around jupyter server.

If one extension (E1) wants to listen to events from another extension (E2), a problem arises is E2 is loaded after E1 is loaded. This is a common use-case. jupyter server doesn't have a dependency injection system yet, so we can't name extension dependencies.

Either way, I think the event logger show allow listeners to be added even if the event isn't registered yet. It's a no-op until the event is registered.

@Zsailer Zsailer added the enhancement New feature or request label Mar 14, 2024
@Zsailer Zsailer force-pushed the pre-flight-listeners branch from 5672f18 to 654f474 Compare March 14, 2024 18:55
@Zsailer Zsailer force-pushed the pre-flight-listeners branch from 654f474 to 7bbacaa Compare March 14, 2024 19:02
@Zsailer
Copy link
Member Author

Zsailer commented Mar 14, 2024

I don't understand why the pre-release tests are failing while all the other tests are passing.

The test that is failing is actually doing what it's suppose to do... raise a SchemaNotFound exception, yet pytest doesn't like it? Not sure what's different about this.

@Zsailer
Copy link
Member Author

Zsailer commented Mar 15, 2024

Found the issue. I didn't realize that SchemaNotRegistered was a warning instead of an exception. Tests are all green now!

Copy link
Member

@davidbrochart davidbrochart left a comment

Choose a reason for hiding this comment

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

Thanks Zach, this looks good!

@Zsailer Zsailer merged commit e7784fd into jupyter:main Mar 18, 2024
23 checks passed
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.

2 participants