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 Kernel.get_parent_header #657

Merged
merged 1 commit into from
May 7, 2021
Merged

Conversation

minrk
Copy link
Member

@minrk minrk commented May 7, 2021

and deprecate instead of break Kernel._parent_header.

While testing the 6.0 alpha, I noticed an issue with the OutputWidget, which references the parent header here. #585 changes _parent_header to be a nested dict of headers, now that two channels may have different parents, causing the output widget to fail with a KeyError on 'msg_id'.

This is ~understandable because widgets are using a private API, but at the same time there isn't a public API for getting the current parent header, which the output widget needs.

I went with:

  • keep Kernel._parent_header to mean shell, but with a deprecation
  • rename new still-private nested headers trait to _parent_headers
  • add Kernel.get_parent_header(channel="shell") API with clearer stability expectations

This way the Output widget doesn't break, and has a clearer path to using a non-private non-deprecated API, which wasn't available before.

related to #635

@blink1073 blink1073 added this to the 6.0 milestone May 7, 2021
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.

Thanks! Heads up, docstring linter picked up one thing

- deprecate Kernel._parent_header
- move new multi-parent header dict to new Kernel._parent_headers instead of changing what Kernel._parent_header means
@minrk minrk force-pushed the public-parent branch from 17a8b61 to 1d89505 Compare May 7, 2021 11:12
@blink1073 blink1073 merged commit c772130 into ipython:master May 7, 2021
@minrk minrk deleted the public-parent branch May 7, 2021 12:23
@minrk
Copy link
Member Author

minrk commented May 7, 2021

Ah, just noticed something weird - _parent_header wasn't actually the parent header, it is the parent message. We should probably fix that while we're at it before we publish a new public API with a name that doesn't make sense.

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.

2 participants