Make ServerProcess and LauncherEntry a HasTraits #521
Conversation
|
@minrk @yuvipanda do either you have time to review this? I know enough about Traitlets to work with them using trial and error, but not enough about the finer details. |
There was a problem hiding this comment.
I think this makes sense, generally.
For generic classes like ServerProcess, it doesn't make sense for values that should be unique like name to be configurable, and I like the kwarg construction which relies only on HasTraits, not specific to Configurable.
For Configurable, only common defaults really make sense in config (e.g. timeout or environment), so if there are none of those, HasTraits makes sense. If there are any, Configurable should probably be kept and instead remove config=True from the fields that don't make sense to be shared.
e.g. this PR currently removes the possibility of setting
c.ServerProcess.timeout = 10for a common default. I'm not sure that is useful or meaningful to remove, so I'm
- +1 on using HasTraits for LauncherEntry
- +1 on removing
config=Trueon most ServerProcess fields that don't make sense as having common overrideable defaults - -0.5 on removing config from ServerProcess altogether, unless we determine that having user-set common defaults really doesn't make sense
| @@ -76,7 +77,7 @@ def _default_path_info(self): | |||
| ) | |||
|
|
|||
|
|
|||
| class ServerProcess(Configurable): | |||
| class ServerProcess(HasTraits): | |||
There was a problem hiding this comment.
This should go with removing all config=True tags from the traits on this class, which might get confusing since that doesn't have any effect anymore.
| help=""" | ||
| Dictionary of processes to supervise & proxy. | ||
|
|
||
| Key should be the name of the process. This is also used by default as | ||
| the URL prefix, and all requests matching this prefix are routed to this process. | ||
|
|
||
| Value should be a dictionary with the following keys: | ||
| Value should be an instance of ``ServerProxy`` or a dictionary with the following keys: |
There was a problem hiding this comment.
| Value should be an instance of ``ServerProxy`` or a dictionary with the following keys: | |
| Value should be an instance of ``ServerProcess`` or a dictionary with the following keys: |
|
Yes, the current commit would remove the possibility to set default values to all proxies. I considered that, but IMHO it is not terribly important. We originally moved to traitlets Configurable to aid with #501. #507 even states, that this was not really meant as a new feature, especially given the fact, that setting defaults like this is not documented atm. But I can see it being a potentially useful side effect, even if I think the applications are niche. But I can move back to Configurable and remove |
|
If there are common config values that make sense, let's keep them and just untag the ones that don't. But if that's not likely (especially if it wasn't intended), I've no objection to going all the way to HasTraits and removing There really is no difference between a Configurable and a HasTraits, except for how they handle traits tagged with |
As I have mentioned in #507 and #501, I do not think there is a good reason to make
ServerProcessandLauncherIconaConfigurable. I like the new approach of using traitlets for these two, butServerProxyis already a Configurable, so I think we should be fine when downgrading them to aHasTraits.I also allowed the entries of
ServerProxy.serversto be an instance ofServerProcess, so users can now doServerProxy.servers[name] = ServerProxy(command=[...], ...)instead of using a dictionary.