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

Restore child_config settings overwritten by directional_link #39

Closed
wants to merge 1 commit into from

Conversation

cexen
Copy link

@cexen cexen commented May 15, 2020

Fixes #30.

@rcthomas
Copy link
Contributor

rcthomas commented Jun 5, 2020

Calling @cmd-ntrf:

@mbmilligan and I were talking about this PR and the associated issue this week on Zoom. The way the directional link works is it overrides the child spawner settings. But if you have a list of profiles, you have to leave the parent setting alone and define it for all of them in the child spawner specification. I think this PR lets the parent setting propagate (so if it's not set for a child it gets defined by the parent) but if it is set, it brings back the child setting.

It works that way but I wondered if you looked at this short PR you might weigh in on whether it looks OK like it breaks anything you intended?

@cmd-ntrf
Copy link
Contributor

cmd-ntrf commented Jun 6, 2020

I think it's ok and does not break what I had in mind with directional links.

However, I would opt for a bit simpler approach: removing child_config keys from the set of keys that are common between the parent and the child when building the set of traits.

So instead of adding an if in the for loop, subtract the keys of self.child_config from the intersection of the parent and the child traits like this:

common_traits = (
  set(self._trait_values.keys()) &
  set(self.child_spawner._trait_values.keys()) - 
  set(self.child_config.keys())
)

@rkdarst
Copy link
Contributor

rkdarst commented Jul 18, 2020

I found I had to use this to make the default_url attribute be passed to the spawner. I haven't looked into much detail about why this is, but without it my general update didn't work at all because of the new way of starting jupyterlab (which requires setting default_url).

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.

Child spawner cmd overridden
4 participants