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 initial cell identifiers awareness #566

Merged
merged 10 commits into from
Apr 12, 2021

Conversation

krassowski
Copy link
Member

@krassowski krassowski commented Feb 27, 2021

To keep the conversation from #553 going, in particular the step 1:

step 1, ignore cell id and make a release with unchanged behavior that is aware of cell ids, as if it were any other opaque metadata field. Since cell IDs are hidden from the user, I think showing them in diffs should also be done at the changed-metadata level, maybe hidden by default.

I am opening a draft PR that might help with conceptualizing what is needed to move forward. The number of failing tests is down from 34 to 32 (fixed test_pretty_print_code_cell and test_git_diff_driver_add_helper_filter I think) but the bulk of merge tests is still failing. They are now failing for a different reason (I temporarily set "/cells/*/id": "remove" while awaiting feedback to highlight the file where the changes are likely needed).

I really wish we could get nbdime out of beta to also allow jupyterlab-git to be properly released and for the lab ecosystem users to migrate to 3.0 and reduce the maintenance burden on the older versions of other Jupyter software too.

This PR might help to close #553.

Copy link
Collaborator

@vidartf vidartf left a comment

Choose a reason for hiding this comment

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

Thanks for picking up the slack here. I have some changes locally that complements this (test changes only). I'll do a PR to your branch with them.

nbdime/merging/notebooks.py Show resolved Hide resolved
nbdime/prettyprint.py Show resolved Hide resolved
@krassowski
Copy link
Member Author

All tests that were failing due to introduction of cell ids are now passing. There are still failing tests, but unrelated:

  • test_git_difftool[jupyter_server]: {'mathjaxUrl': ''} != {'mathjaxUrl': '/static/components/MathJax/MathJax.js'}
  • test_offline_mathjax: tornado.httpclient.HTTPClientError: HTTP 404: Not Found

@krassowski
Copy link
Member Author

Comparing jupyter_server/search?q=mathjax_url and notebook/search?q=mathjax_url it looks that:

Ideally, we would want to check that ids are properly merged/removed, but this is ok as a stop-gap to get tests running
@vidartf
Copy link
Collaborator

vidartf commented Apr 12, 2021

Thanks! 🥇

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.

Support cell IDs
2 participants