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

Workaround for later initialization, recovers document_ttl #318

Closed
wants to merge 4 commits into from

Conversation

asteppke
Copy link
Contributor

This is a short fix for #315.

In the future this can be updated when the changed traitlets infrastructure is fully taken into account but for now this restores the document_ttl functionality and should have no negative side effects.

Copy link
Contributor

Binder 👈 Launch a Binder on branch asteppke/jupyter-collaboration/main

@davidbrochart
Copy link
Collaborator

Thanks @asteppke.

In the future this can be updated when the changed traitlets infrastructure is fully taken into account

What do you mean by "changed traitlets infrastructure"?

def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
# Class is instantiated later, so we need to set the document_ttl here
self.document_ttl = int(self.document_ttl) if self.document_ttl is not None else None
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand what this changes. Could you write a test that shows that this fixes something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a test that checks whether the store has the correct parameter from the command line arguments.

Without the changeset above the store writing fails immediately if document_ttl is set.

@asteppke
Copy link
Contributor Author

Thanks @asteppke.

In the future this can be updated when the changed traitlets infrastructure is fully taken into account

What do you mean by "changed traitlets infrastructure"?

I did not bisect this in detail but when you implemented the document_ttl functionality in the past I think it worked as intended. Instead of going through the normal traitlets initialization code the attributes are set directly here in jupyter_collaboration (I guess because of late initialization). The traitlet guys mention in passing in their documentation that since 5.0 there is a deferral happening of the conversion from command line arguments to traits. So therefore jupyter_collaboration is now setting the 'still-unconverted-strings' as attributes instead of traits.

So this adding the missing conversion step. I can see if this can be shown also within the a test in the test suite.

@asteppke
Copy link
Contributor Author

I added the corresponding tests for the document_ttl. Other unrelated tests fail though already without these commits.

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