Closed batpad closed 1 year ago
Wieeee this looks nice!
There is a coupling between the new config option and choices.*.display_name
and I'd love if that is captured directly by the naming of the config. In words, I'm describing the option as "the display_name
of an entry added to choices
to choose an unlisted choice". That makes me lean towards a naming like:
display_name_in_choices
choices_entry_display_name
I think it sounds fine with display_name_in_choices
.
For reference, this is how the config may look
'profile_options': {
'image': {
'display_name': 'Image',
'choices': {
'base': {
'display_name': 'jupyter/base-notebook:latest',
'kubespawner_override': {
'image': 'jupyter/base-notebook:latest'
},
},
'minimal': {
'display_name': 'jupyter/minimal-notebook:latest',
'default': True,
'kubespawner_override': {
'image': 'jupyter/minimal-notebook:latest'
},
},
},
'unlisted_choice': {
'enabled': True,
'display_name': 'Other image',
'display_name_in_choices': 'Enter image manually',
'validation_regex': '^jupyter/.+:.+$',
'validation_message': 'Must be an image matching ^jupyter/<name>:<tag>$',
'kubespawner_override': {'image': '{value}'},
},
},
},
Maybe we should reduce complexity like below in the jinja2 template parsing a profile_list
variable by initializing profile_list
more thoroughly in _get_initialized_profile_list
.
{%- if option['unlisted_choice']['other_text'] %}
{{ option['unlisted_choice']['other_text'] }}
{%- else %}
Other...
{%- endif %}
If we in _get_initialized_profile_list
provide a default for each profile_options
entry with unlisted_choice.enabled=False
and unlisted_choice.display_name_in_choices="Other..."
we would be able to reduce the complexity quite a bit.
{{ option['unlisted_choice']['other_text'] }}
(I fixed a mixup between #756 and #757 in the title and PR description)
(I fixed a mixup between https://github.com/jupyterhub/kubespawner/issues/756 and https://github.com/jupyterhub/kubespawner/issues/757 in the title and PR description)
Yikes, so sorry about that, and thanks for fixing. Apologies the branch is still named 756-custom-other-text
- let me know if you would like me to make a new PR with a new branch name there.
I think it sounds fine with display_name_in_choices
Sounds good to me! Pushed the change.
If we in _get_initialized_profile_list provide a default for each profile_options entry with unlisted_choice.enabled=False and unlisted_choice.display_name_in_choices="Other..." we would be able to reduce the complexity quite a bit.
Have tried to do this with the latest commit. It seems to work, but would be great to get your eyes on those bits and if it lines up with how you were envisioning this.
Thanks so much for the quick and thoughtful review!
Am taking a look at the failing test.
let me know if you would like me to make a new PR with a new branch name there.
No no worries, I think that shouldn't matter for anyone in any way. You could do git rebase -i main
and reword the first commit if you want, I can see how that could possibly cause confusion.
Have tried to do this with the latest commit. It seems to work, but would be great to get your eyes on those bits and if it lines up with how you were envisioning this.
Thanks! This section was added, and its something like that I was thinking.
if option_config.get('unlisted_choice'):
if not 'display_name_in_choices' in option_config.get(
'unlisted_choice'
):
option_config['unlisted_choice'][
'display_name_in_choices'
] = "Other..."
I think its important that we initialize unlisted_choice as a dictionary if needed as well, and I think we will check enabled
from the template quite often, so then that should probably be initialized at least also.
if option_config.get("unlisted_choice") is None:
option_config["unlisted_choice"] = {}
if option_config["unlisted_choice"].get("enabled") is None:
option_config["unlisted_choice"]["enabled"] = False
if option_config["unlisted_choice"].get("display_name_in_choices") is None:
option_config["unlisted_choice"]["display_name_in_choices"] = "Other..."
I remembered now that there is a smoother way of writing this also, I suggest going with this!
unlisted_choice = option_config.setdefault("unlisted_choice", {})
unlisted_choice.setdefault("enabled", False)
unlisted_choice.setdefault("display_name_in_choices", "Other...")
There is also a comment above where this logic resides that should be updated to reflect that default values for unlisted_choice is populated as well.
Looking into failing tests.
@consideRatio made a small change to the setdefault
stuff to only set display_name_in_choices
if unlisted_choice
is enabled
. It seemed a bit awkward to get that set even when unlisted_choice
was disabled (and made the test a bit confusing) - but there's no strong opinion here, also happy to change the test to expect display_name_in_choices
to be Other...
after setting of defaults even when unlisted_choice.enabled
is False
.
On your end @batpad, is this ready to be merged? If so, I suggest we go for it!
PR Summary
The profile's in
profile_list
can provideprofile_options
, for example to choose a pre-defined image from a list or opt to specify anunlisted_choice
- not part of this list. This pr adds theunlisted_choice.display_name_in_choices
configuration to specify how the option/choice added to the list of pre-defined entries should be presented.Before, it looked like:
After, it can be configured to look like:
cc @yuvipanda @consideRatio @GeorgianaElena
757 should be a bit more straightforward than #756, so made this a separate smaller PR, correctly updated with latest
main
.Here we just add an option to the
unlisted_choice
options dictionary and output that in the select instead ofOther...
if present.I have called that option
other_text
, but that doesn't sound great to me, so if anyone has a better recommendation for naming, please let know / go ahead and make the change.I'm not sure if I missed adding documentation for this new option somewhere.
Many apologies for the delay getting to this, please let me know if this looks fine. I'll make a separate PR toward #756.
Thank you!