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

ListSelector default #817

Merged
merged 7 commits into from
Aug 17, 2023
Merged

ListSelector default #817

merged 7 commits into from
Aug 17, 2023

Conversation

jbednar
Copy link
Member

@jbednar jbednar commented Aug 17, 2023

Fixes #807. Makes _update_state from #794 aware of list-type defaults so that it adds the items on the list to the objects, not the entire default value as a sublist. This change does seem to be successful, avoiding objects=1, 2, 3, [1, 2]]:

image

Regarding the discussion in #807:

Actually I'm even thinking that #693 is a silly usage of Param and I'm no longer sure we should "fix" it. That's the example I shared in #693, why would you not include 3 in the objects?? On top of that check_on_set is pretty clearly worded with on_set and not on_init.

After looking into it I agree. We definitely shouldn't do any extra work to make this code legal:

import param

class P(param.Parameterized):
    s = param.Selector(default=3, objects=[1, 2], check_on_set=False)

p = P()

If people wanted 3 on the objects list, they should have put it there, since they supplied an objects list. This code is currently accepted by this PR, yielding p.params.objects=[1, 2, 3], but if it weren't, I'd be equally happy.

Less clear is what should happen when no objects list is supplied:

image

In this case, it does seem appropriate to be adding the default value to the objects list, and supporting this case automatically makes the above less-important case work.

The way it works still depends on Maxime's _update_state approach, which I can now appreciate. I had previously suggested trying to eliminate _update_state, but the problem (of which I'm sure @maximlt is aware) is that using the Sentinel approach for slot values means that the _objects list doesn't get a concrete value until after the constructor completes and returns control to the metaclass. By that point it's too late for the constructor to add items to the list based on the default, hence needing a new _update_state method to fill it in later.

So if we want to eliminate _update_state (which is a worthwhile goal), I think we'd have to require the objects list to include the default value, which seems like a big disruption to previous usage. If Panel developers are ok with that, sure, but otherwise I guess we are stuck with _update_state.

@jbednar jbednar requested review from philippjfr and maximlt August 17, 2023 03:27
Copy link
Member

@philippjfr philippjfr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@philippjfr philippjfr merged commit 83e57ea into main Aug 17, 2023
@philippjfr philippjfr deleted the listselectordefault branch August 17, 2023 09:55
@jbednar jbednar changed the title Listselectordefault ListSelector default Aug 25, 2023
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.

ListSelector: regression when check_on_set=False
2 participants