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

switch to jupyter_events #862

Merged
merged 8 commits into from
Aug 29, 2022
Merged

Conversation

Zsailer
Copy link
Member

@Zsailer Zsailer commented May 31, 2022

Replaces jupyter_telemetry with jupyter_events.

Telemetry is a more specific use case of the Jupyter Event System mentioned in #780, and it brings some extra constraints that won't be addressed by the (currently available) "event system" use-case. Telemetry might be added to Jupyter Server at a later time.

@codecov-commenter
Copy link

codecov-commenter commented May 31, 2022

Codecov Report

Merging #862 (0028483) into main (0028483) will not change coverage.
The diff coverage is n/a.

❗ Current head 0028483 differs from pull request most recent head 2f7ef4b. Consider uploading reports for the commit 2f7ef4b to get more accurate results

@@           Coverage Diff           @@
##             main     #862   +/-   ##
=======================================
  Coverage   72.12%   72.12%           
=======================================
  Files          65       65           
  Lines        8044     8044           
  Branches     1343     1343           
=======================================
  Hits         5802     5802           
  Misses       1836     1836           
  Partials      406      406           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Zsailer
Copy link
Member Author

Zsailer commented May 31, 2022

@3coins and @kiendang—pinging you to let you know I've move most of the event handling logic from jupyter_telemetry into jupyter_events. We can start to deprecate these things in jupyter_telemetry.

@Zsailer
Copy link
Member Author

Zsailer commented Jun 6, 2022

@jupyter-server/jupyter-server-council this should be ready to go. Would someone mind reviewing+merging?

@blink1073
Copy link
Contributor

Are we not renaming "categories" first?

@Zsailer
Copy link
Member Author

Zsailer commented Jun 6, 2022

Ah, right. Yeah, let's settle that in jupyter_events before merging here. Thanks, @blink1073! I'll open a PR there later today.

@dlqqq
Copy link
Contributor

dlqqq commented Aug 29, 2022

Looks good! The changes are pretty lean, so I say we can just merge this after the pipeline passes.

Took a quick look at the failing tests here: https://github.com/jupyter-server/jupyter_server/runs/8020778443?check_suite_focus=true. It looks like there's an error when trying to dereference __class__.__name__. Likely something wrong with the test fixtures?

@dlqqq
Copy link
Contributor

dlqqq commented Aug 29, 2022

@Zsailer BTW, have we established a convention for where to place event schemas within the jupyter_server project? I was thinking just under /jupyter_server/schemas. Could you also paste the Contents Manager event schema you showed off during contributing hour into a Github Gist as an example for developers to refer to?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants