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

Make kernel message typings more correct #6433

Merged
merged 4 commits into from Jun 8, 2019

Conversation

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented May 29, 2019

References

This continues work done in #6412.

jupyter/jupyter_client#442 and jupyter/jupyter_client#443 stemmed from the investigations into the spec for this PR.

Code changes

The idea here was to simplify the typings, and more fully conform to the spec about error reply messages.

User-facing changes

None

Backwards-incompatible changes

Kernel message typings are changed in a backwards-incompatible way, since we are eliminating some publicly available interface names. Some typings, for example, are now accessible via the message content type fields (e.g., ICompleteRequestMsg['content'])

@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented May 29, 2019

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Jun 6, 2019

My plan is to work on this more tomorrow (Friday) to try to have it in shape for 1.0RC, or bump it if I can't get it in shape tomorrow.

jasongrout added 4 commits Jun 7, 2019
All kernel message replies have a status field. This ensures that reply messages reflect this and handle the abort and error status appropriately.

We also simplify typings since we can refer to most content typings using the field accessor.
@ian-r-rose figured out this fix. This may be already in master and the commit could be removed.
@jasongrout jasongrout marked this pull request as ready for review Jun 7, 2019
@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Jun 7, 2019

I think this is ready now. CC @blink1073 @ian-r-rose

@jasongrout jasongrout changed the title WIP: Eliminate various kernel message content typings. Make kernel message typings more correct Jun 7, 2019
Copy link
Member

@blink1073 blink1073 left a comment

Nice one, thanks!

@blink1073 blink1073 merged commit bd04328 into jupyterlab:master Jun 8, 2019
9 checks passed
SylvainCorlay
Copy link
Member

SylvainCorlay commented on 726ce5e Jun 18, 2019

Choose a reason for hiding this comment

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

@jasongrout this is a backward incompatible change to @jupyterlab/services.

I have a working update in ipywidgets/jupyterlab-manager but ideally, we should export ReplyContent and ICommInfoReply for clients to @jupyterlab/services.

jasongrout
Copy link
Contributor

jasongrout commented on 726ce5e Jun 18, 2019

Choose a reason for hiding this comment

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

You can get the comm reply content with ICommInfoReplyMsg['content'] - is that what you want?

@vidartf
Copy link
Member

@vidartf vidartf commented Jun 18, 2019

I think he meant that dropping the typings will break TS projects that use those names in their own code.

Adding export type ICommInfoReply = ICommInfoReplyMsg['content'] would solve this, right?

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Jun 18, 2019

No, the typings have changed to handle error replies. I think it's okay to have a breaking change in the major release that simplifies the exported types and makes them more correct as well.

@lock
Copy link

@lock lock bot commented Aug 6, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related discussion.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants