-
Notifications
You must be signed in to change notification settings - Fork 301
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 notebook_dir as trait to be deprecated #36
Conversation
jupyter_server/serverapp.py
Outdated
@validate('notebook_dir') | ||
def _update_notebook_dir(self, change): | ||
self.log.warning(_("\n notebook_dir is deprecated, use root_dir.\n")) | ||
self.root_dir = change['value'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
With this behavior, there may be an issue when setting
root_dir
, and reading notebook_dir. -
Maybe an observer on
root_dir
setting thenotebook_dir
trait through set_trait (bypassing validation) would be better. -
However, we should be careful about incompatible setting of one vs the other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, that should be an observe
decorator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, you're right. If root_dir
changes, notebook_dir
doesn't change.
I think setting notebook_dir
in the root_dir
observer should fix that. Why bypass validation? They should end up having the same validation logic right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about adding a
if self.notebook_dir != change['new']:
self.notebook_dir = change['new']
to the root_dir
observer, and
if self.root_dir != change['new']:
self.root_dir = change['new']
to the notebook_dir
observer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ensures the traits are equal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to do some experiments. But it is not trivial.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
totally agree it's not trivial.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the check above if you want to test it. Just checks notebook_dir
and root_dir
are equal. If not, update the other trait.
02eb6f6
to
a6f976b
Compare
The pattern used in the classic notebook is to observe the deprecated trait for changes; when it is changed 1) warn the user and 2) allow it to change the new trait. Changing the new trait does not update the deprecated trait. Two examples in the classic notebook are Should I simply follow that pattern here? @observe('notebook_dir')
def _update_notebook_dir(self, change):
self.log.warning(_('deprecation_warning'))
self.root_dir = change['new'] |
I think we can do that. |
Will deprecating and then eventually removing JupyterHub uses it in a pretty essential way, since that's what |
@krinsman. We'll have to change this trait in JupyterHub, too. |
👍 @SylvainCorlay merging. |
This fixes issue #35.