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 sure display_data always contain metadata #269

Merged
merged 1 commit into from
Jul 15, 2021

Conversation

martinRenou
Copy link
Member

@martinRenou martinRenou commented Jul 15, 2021

References

Attempt to fix #264

works

@jtpio jtpio added the enhancement New feature or request label Jul 15, 2021
@jtpio
Copy link
Member

jtpio commented Jul 15, 2021

Thanks!

Looks like having that default should indeed be fine anyway.

@martinRenou
Copy link
Member Author

(Edited the top comment with a screenshot, that seems to work)

@martinRenou
Copy link
Member Author

Checking the other notebooks before making this non-draft

@bollwyvl
Copy link
Collaborator

seems legit!

@martinRenou
Copy link
Member Author

martinRenou commented Jul 15, 2021

I honestly believe we shouldn't do this, and that py3dmol should not use IPython.display.publish_display_data directly. But if ipykernel is resilient with this, then maybe pyolite should be too.

@martinRenou martinRenou marked this pull request as ready for review July 15, 2021 13:07
Copy link
Member

@jtpio jtpio left a comment

Choose a reason for hiding this comment

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

Thanks!

@jtpio jtpio merged commit 15f2c0f into jupyterlite:main Jul 15, 2021
@martinRenou martinRenou deleted the displaydata_metadata branch July 15, 2021 13:26
@psychemedia
Copy link
Contributor

@martinRenou Re: "I honestly believe we shouldn't do this, and that py3dmol should not use IPython.display.publish_display_data directly." Is there an approach you think they should take? I am happen to open an issue on their repo "observing" that their perhaps atypical/non-standard(?) approach caused this project to have to error trap on what appears to be an exceptional case so far.

@martinRenou
Copy link
Member Author

I am not 100% of this. But this publish_display_data seems somewhat like a private API to me. I would suggest using display(obj, raw=True) instead.

@martinRenou
Copy link
Member Author

Although this is documented API https://ipython.readthedocs.io/en/stable/api/generated/IPython.display.html#IPython.display.publish_display_data. So maybe it's an issue on IPython's side which should set the metadata to an empty dict by default.

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.

Problem running py3DMol - widget using publish_display_data() doesn't display
4 participants