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

Make config.ServerProcess into a Configurable #507

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

manics
Copy link
Member

@manics manics commented Oct 24, 2024

This isn't useful in the jupyter-server-extension, but it should be useful in
#501 (review)

An added benefit is this makes it easier to add more parameters in future since the declaration of the class and the handling of defaults is now in the same place.

@jwindgassen
Copy link

jwindgassen commented Oct 24, 2024

While at it, we should probably also convert LauncherEntry. Feel free to copy it from my implementation

Edit: I think the self.parent might not work for that. But I don't have much experience with traitlets

@manics
Copy link
Member Author

manics commented Oct 24, 2024

Thanks, I had LauncherEntry in my local branch, but was hoping to minimise changes. As it happens making it Configurable is the easiest way to set the defaults.

Assuming ServerProcess can be configured externally I don't think LauncherEntry needs to be, it can be initialised from a dict as at present.

@manics
Copy link
Member Author

manics commented Oct 27, 2024

A nice benefit of this is we can autogenerate some of the manually written docs:
#508

@jwindgassen
Copy link

jwindgassen commented Nov 12, 2024

I see why you didn't want to use Configurable for the Launcher Entry. But I think you could just switch to HasTraits as a base class, this should still work for default values. Or make it consistent and make both only use HasTraits, as they are both only used in the ServerProxy class. I think I should still be able to use them in a configurable in my code.

Is there anything else still required here? It would be nice to merge this, so I can update my code it in #501.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants