Eliminating `_update_state`? about param

[maximlt]

Thanks for the great write-up! It's an exact recollection of what happened and why I had to add _update_state, to deal with dynamically computed Parameter attributes in an inheritance context.

I'd be in favor of requiring users to specific check_on_set and making that change now before Param 2.0:

• check_on_set isn't used so much, the effect of this change isn't going to have a large impact
• a Selector without any objects value and an implicit check_on_set=False is a bit of a weird case, as this disables validation entirely. I actually suspect some users start with an empty list of objects and populate it afterwards (e.g. from a database query), without realizing that check_on_set has automatically been set to False.

Dealing with allow_None is a lot more controversial, having it set automatically when default=None is quite handy and is used all over the place, with dozens and dozens of occurrences in HoloViews and Panel. I would actually not mind having to be explicit, I'm on the fence though about making such an impactful change without any deprecation period.

To complete the list of attributes than can be dynamically computed, there's alsoTuple.length from the length of the default value, unless it's None in which case it must be provided explicitly.

[philippjfr]

No my misunderstanding, that all sounds fine. In the end _update_state is just an implementation detail though so no opinion whether to remove it, if it gets eliminated by making those changes then that's good too.

[maximlt]

I'm going to try to remove _update_state now, making check_on_set static. I wouldn't be so sure that it is the only reason for _update_state to exist. Getting the Selector Parameter, and its subclasses, to work in an inheritance context is definitely not easy. _update_state is an undocumented private method, if I don't manage to get rid of it in a reasonable amount of time, I'm fine releasing Param 2.0 with that, since anyway the release notes won't mention it and it can be removed anytime. Of course, the sooner the better if it's meant to be removed.

[jbednar]

I think the Tuple length imputation is safe and reasonably clean, so it sounds like @maximlt votes for changing check_on_set to have a fixed default of True, but to keep allow_None imputation as it is. Ok by me; my concern is about disrupting current usage, and if representatives of Panel and HoloViews are ok with that, ok by me! @philippjfr ?

[philippjfr]

Thanks for the detailed writeups both of you. I fully agree with @maximlt's summary and agree that making check_on_set be an explicit setting is fine but allow_None would be far too disruptive. My only other comment is that I really don't understand the naming of check_on_set and I wish we could call it something more sensible.

[jbednar]

Ok, sounds good. That option controls whether a given value is validated against the list of allowed values, i.e., it checks that the value is allowed when someone sets the value. Suggestions for another name, now that people will need to learn about this option?

[philippjfr]

Not sure I have anything good. In the Panel context this will line up with Autocomplete and MultiChoice widgets which allow either restricting the options or adding new options. There it's called restrict but that isn't really clear enough in this context, restrict_objects could be okay, add_on_set is slightly clearer than check_on_set but also not great, add_unseen is clearer but I don't like it. I got nothing good.

[jbednar]

That's an argument for keeping the status quo, but if anyone does have a really clear name, we can add that as an alias when we come up with it and make our docs focus on that instead.

[philippjfr]

Agreed, my vote is to take no action on renaming check_on_set or any major reworking of _update_state for the 2.0 release.

[jbednar]

We're agreed about keeping the name of check_on_set, but my understanding is that above you were agreeing with simplifying its semantics, and removing _update_state is a consequence of that simplification. I.e. _update_state exists only to achieve those semantics, and we would not need preserve it otherwise. And as _update_state has not ever been released, now is the time to delete it, if we are going to delete it. Unless I got confused somewhere?

[jbednar]

It's undocumented, but if it's required for any user-visible behavior, better for us to find that out now!

[maximlt]

Also, I get the idea to clean up some method if we can. But providing a way to update the state of a Parameter doesn't sound too bad to me. Parameters could all be lazily created and be finalized when bound to a Parameterized class.

[jbednar]

Sure. On balance if we don't need that I want to prioritize someone being able to read Param's code and understand what happens when, which is quite difficult given the Parameter itself, its metaclass, Parameterized, and its metaclass, so anything that can be omitted from that control flow is a win.