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

Modify https://github.com/jupyter-server/jupyter_server/pull/1367 #1

Conversation

fcollonval
Copy link
Owner

This modifies jupyter-server#1367 in order to:

  • allow only a valid hash algorithm from hashlib available algorithms
  • don't require a new attribute on ContentsManager as contents manager are expected to update to the latest API supporting hash
  • Make Mixin classes backward compatible as they may be reused downstream
  • Don't change the purpose of _read_file (it only read files)
  • Limit the number of new methods (only adds _get_hash)
  • Don't set hash_algorithm except if hash is set
  • Don't set the content if we only want the hash (it will avoid encoding the bytes for example)
  • Don't read twice the content for notebooks

@Wh1isper feel free to cherry-pick my commits to your branch. I would strongly suggest removing the downstream test against jupytext because we have already very limited maintenance capability.

)
validate_model(model, expect_content=content, expect_md5=md5)
except TypeError:
# Fallback for ContentsManager not handling the require_hash argument

Choose a reason for hiding this comment

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

This is a good fallback method. I had thought about using inspect.signature and I just want to get your opinion.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I thought about doing that once. But as the handler are instantiated for each request, I think the try/except is faster than signature inspection.

Wh1isper added a commit to Wh1isper/jupyter_server that referenced this pull request Nov 23, 2023
Co-authored-by: Frédéric Collonval <fcollonval@gmail.com>
@fcollonval fcollonval closed this Nov 24, 2023
@fcollonval fcollonval deleted the fix/1366-Latest-release-is-breaking-custom-ContentManager branch November 24, 2023 13:42
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.

2 participants