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 'name' flag to workspaces import CLI #5695

Merged
merged 4 commits into from Jan 8, 2019

Conversation

@Zsailer
Copy link
Member

@Zsailer Zsailer commented Nov 28, 2018

Fixes #5694

This adds a --name argument to the jupyter lab workspaces import command. The workspace name can now be set from the command line.

@Zsailer Zsailer force-pushed the workspace-flag branch from 7a5241b to 1e29db7 Nov 28, 2018
@ellisonbg ellisonbg added this to the 1.0 milestone Nov 29, 2018
@Zsailer Zsailer requested a review from ian-r-rose Nov 29, 2018
@ian-r-rose ian-r-rose self-assigned this Nov 29, 2018
if 'metadata' not in workspace:
raise Exception('The `metadata` field is missing.')
# See if workspace name is given, inject into workspace.
if self.workspace_name != '':
Copy link
Member

@ian-r-rose ian-r-rose Nov 30, 2018

Choose a reason for hiding this comment

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

I think we also want another branch here for if the --name argument is given as the empty string. In that case the workspace id would just be that of the default JupyterLab workspace (i.e. /lab). I'm not sure the best way to accomplish that via the CLI, though. Can we distinguish between None and "" using this traitlets-based approach?

Copy link
Member Author

@Zsailer Zsailer Nov 30, 2018

Choose a reason for hiding this comment

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

Can we distinguish between None and "" using this traitlets-based approach?

I don't think so. I think None is an invalid type for a Unicode trait.

Copy link
Member Author

@Zsailer Zsailer Nov 30, 2018

Choose a reason for hiding this comment

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

We can catch an empty string though and turn it into the default workspace.

Copy link
Member Author

@Zsailer Zsailer Dec 3, 2018

Choose a reason for hiding this comment

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

Actually, I just found out that Traits have a keyword argument allow_none. This should solve the problem. Updating the PR now.

Copy link
Member

@ian-r-rose ian-r-rose Dec 5, 2018

Choose a reason for hiding this comment

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

I'm don't that this will fix the entire problem. The UI I am thinking of is something like

  1. If no --name arg is given, then the importer uses the same ID that it currently has.
  2. If a --name="foo" is given, the workspace ID becomes /user/username/lab/workspaces/foo.
  3. If a --name="" is given, the workspace ID becomes that of the default workspace, i.e. /user/username/lab.

I don't think that the current behavior will hit case number 3. That is to say, we need special logic for the default workspace URL since it is different from all the named workspace URLs.

I don't have too strong of a grasp of how the traitlets command-line processing happens -- is a distinction made between --name="" vs --name=?

Copy link
Member Author

@Zsailer Zsailer Dec 6, 2018

Choose a reason for hiding this comment

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

is a distinction made between --name="" vs --name=?

No. traitlets assumes the empty space after --name= is an empty string. To get None, the user has to explicitly pass --name=None or not use --name.

if self.workspace_name is not None:
workspace_id = ujoin(base_url, workspaces_url, self.workspace_name)
if self.workspace_name == "":
workspace_id = ujoin(base_url, 'lab')
Copy link
Member Author

@Zsailer Zsailer Dec 6, 2018

Choose a reason for hiding this comment

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

@ian-r-rose

Okay, here is the updated logic to catch --name="". The workspace_id becomes the default /lab endpoint.

Copy link
Member

@afshin afshin Dec 20, 2018

Choose a reason for hiding this comment

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

I think this should be workspace_id = ujoin(base_url, page_url)

cf. line 244 below

Copy link
Member Author

@Zsailer Zsailer Dec 21, 2018

Choose a reason for hiding this comment

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

👍 Fixed!

if self.workspace_name is not None:
workspace_id = ujoin(base_url, workspaces_url, self.workspace_name)
if self.workspace_name == "":
workspace_id = ujoin(base_url, 'lab')
Copy link
Member

@afshin afshin Dec 20, 2018

Choose a reason for hiding this comment

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

I think this should be workspace_id = ujoin(base_url, page_url)

cf. line 244 below

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Jan 8, 2019

Thanks @Zsailer, and sorry for the slow turnaround. Works great!

@ian-r-rose ian-r-rose merged commit 44c0a90 into jupyterlab:master Jan 8, 2019
3 checks passed
@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants