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

Custom kernel message serializer #15254

Merged
merged 3 commits into from Dec 12, 2023

Conversation

DonJayamanne
Copy link
Contributor

@DonJayamanne DonJayamanne commented Oct 11, 2023

References

Fixes #15198

Code changes

Exposes the message serializer in the ISettings, allowing one to customize how messages are serialized/deserialized based on the server (settings).

User-facing changes

N/A

Backwards-incompatible changes

N/A

@jupyterlab-probot
Copy link

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link: Binder

Copy link
Member

@krassowski krassowski left a comment

Choose a reason for hiding this comment

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

Thanks @DonJayamanne, this looks good to me!

@DonJayamanne DonJayamanne marked this pull request as ready for review October 11, 2023 20:56
@DonJayamanne DonJayamanne marked this pull request as draft October 11, 2023 20:56
@DonJayamanne
Copy link
Contributor Author

@krassowski
Should I mark this as ready for review, or is there something else we need?
I do not think we need tests for this change, and the docs are generated from the ts code.

@krassowski
Copy link
Member

krassowski commented Oct 11, 2023

Addding a unit test ensuring that the serializer can actually be changed can help catch regression if this function gets rewritten (all it takes to break it is to move ...options one line up) so it could benefit you in the future. Other than that no follow up tasks here.

@fcollonval fcollonval marked this pull request as ready for review October 30, 2023 09:24
@fcollonval fcollonval added this to the 4.1.0 milestone Oct 30, 2023
Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks @DonJayamanne

I agree that it will be nice to test providing custom de/serializers in a test.

@krassowski krassowski merged commit 9844a6f into jupyterlab:main Dec 12, 2023
73 of 77 checks passed
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.

Support to override the serializer used in kernel.
3 participants