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

feat: implement Output widget that mimics a frontend #68

Merged

Conversation

maartenbreddels
Copy link
Contributor

This is a port of voila-dashboards/voila#91 and subsequent fixes.

@maartenbreddels
Copy link
Contributor Author

Fixes #24

I've added a changelog entry and bumped the version number. It's a new workflow I'm using, which makes it easier to track changes (and keep semantic versioning).
Happy to remove it if people don't like it though.

@maartenbreddels maartenbreddels force-pushed the feat_mimic_output_widget branch 2 times, most recently from 8a994e8 to 6f2cee2 Compare May 26, 2020 08:56
@maartenbreddels
Copy link
Contributor Author

Failure at https://travis-ci.org/github/jupyter/nbclient/jobs/691227550 seems unrelated, but a bit worrying.

@davidbrochart
Copy link
Member

This test fails randomly, I know it's a problem. I'm restarting it.

@davidbrochart
Copy link
Member

Now it passes 😄

nbclient/_version.py Outdated Show resolved Hide resolved
nbclient/client.py Outdated Show resolved Hide resolved
data = content['data']
state = data['state']
comm_id = msg['content']['comm_id']
if state['_model_module'] == '@jupyter-widgets/output' and\
Copy link
Member

Choose a reason for hiding this comment

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

Same here, probably filter by target name.

@maartenbreddels maartenbreddels force-pushed the feat_mimic_output_widget branch 2 times, most recently from 13a91d1 to af7959b Compare May 26, 2020 10:56
@maartenbreddels
Copy link
Contributor Author

docs fail because of missing ipykernel dependency:
ModuleNotFoundError: No module named 'ipykernel'

Not sure how to handle this, we probably don't want or need to depend on it right?

It is only needed for from ipykernel.jsonutil import json_clean Maybe some other package has this as well.

@davidbrochart
Copy link
Member

I think @choldgraf should be able to help us here.

maartenbreddels added a commit to maartenbreddels/voila that referenced this pull request May 26, 2020
maartenbreddels added a commit to maartenbreddels/voila that referenced this pull request May 26, 2020
maartenbreddels added a commit to maartenbreddels/voila that referenced this pull request May 26, 2020
maartenbreddels added a commit to maartenbreddels/voila that referenced this pull request May 26, 2020
maartenbreddels added a commit to maartenbreddels/voila that referenced this pull request May 26, 2020
@@ -0,0 +1,85 @@
from ipykernel.jsonutil import json_clean
Copy link
Member

Choose a reason for hiding this comment

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

Indeed, this seems like a weak reason to depend on ipykernel.

Should we duplicate the implementation of json_clean in this package instead?

Copy link
Member

Choose a reason for hiding this comment

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

There is a jsonutils in jupyter_client as well but without that function.

How about adding a jsonutils here?

@maartenbreddels
Copy link
Contributor Author

I added and blackified the jsontil.py, we may want to find a better long term solution some day.

@SylvainCorlay SylvainCorlay merged commit 35ff13d into jupyter:master May 26, 2020
@SylvainCorlay
Copy link
Member

Yes! Really happy to see things go through.

@maartenbreddels maartenbreddels deleted the feat_mimic_output_widget branch May 26, 2020 14:45
@SylvainCorlay
Copy link
Member

@choldgraf would you be fine with releasing 0.4.0 now?

# unanmbiguous binary data is base64-encoded
# (this probably should have happened upstream)
return b2a_base64(obj).decode('ascii')
else:
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't need this code branch

@MSeal
Copy link
Contributor

MSeal commented May 26, 2020

I added and blackified the jsontil.py, we may want to find a better long term solution some day.

Yes that did seem like overkill, but if it's coming from a working solution we can use what works until someone refactors later.

@choldgraf would you be fine with releasing 0.4.0 now?

Let's double check what's still open in issues / PRs first. I added you as a maintainer on PyPI @SylvainCorlay

@choldgraf
Copy link
Contributor

this is super exciting :-) thanks @maartenbreddels and @SylvainCorlay for pushing this through!

@choldgraf
Copy link
Contributor

I am +1 on pushing out a new release unless there's an obvious blocker in our issues or a super low-hanging fruit

@MSeal
Copy link
Contributor

MSeal commented May 26, 2020

I think #64 and maybe #58 could be addressed before a release. We're still in sub 1.0 releases so it's also ok for us to ship now and prep a 0.5 for those other two.

On that note we should set a goal for 1.0 release -- it's probably pretty close? I'll start an issue to track what we want to target to be considered feature complete

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

Successfully merging this pull request may close these issues.

5 participants