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

Merged
merged 5 commits into from
Dec 4, 2024

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

@manics manics requested a review from yuvipanda November 6, 2024 23:29
@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.

@manics
Copy link
Member Author

manics commented Dec 2, 2024

@yuvipanda what do you think of this?

@ryanlovett
Copy link
Collaborator

@manics I like how the new class relieves us of having to manually specify ServerProcess' fields. The docs generation is also nice.

Will merge per request.

@ryanlovett ryanlovett merged commit 76a98c9 into jupyterhub:main Dec 4, 2024
15 checks passed
@manics manics deleted the config-traitlets branch December 4, 2024 21:37
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.

3 participants