Drop support for Mapping types as Selection options

[jtpio]

Fixes #1958.
Continuation of #2130.

Breaking change

• This removes support for Mapping types when constructing a new Dropdown.

[pbugnion]

Thanks!

Currently, the options traitlet has type Any(). I wonder if we can change it to a traitlets.List or some such?

I think we can probably cut more code from _Selection in general.

[jtpio]

That's a good point, let's specify the type even more.

[jtpio]

Currently, the options traitlet has type Any(). I wonder if we can change it to a traitlets.List or some such?

traitlets.List might be slightly too specific, since options should also support other iterables such as Tuple.

[pbugnion]

There is no reasonable traitlet supertype that includes general Python iterables, as far as I can see.

For instance, the following currently works:

import numpy as np
options = np.array(['hello', 'world'])
widgets.Dropdown(options=options)

As far as I can tell, no refinement of Any would let us keep this behaviour, so I vote in favour of keeping Any.

[pbugnion]

I think some of the entries in doc_snippets also need updating (e.g. multiple_selection_params).

[jtpio]

I think some of the entries in doc_snippets also need updating (e.g. multiple_selection_params).

For the _MultipleSelection, don't we want to keep support for Mapping / dict? At least for now, and potentially drop it in a separate PR.

[pbugnion]

For the _MultipleSelection, don't we want to keep support for Mapping / dict? At least for now, and potentially drop it in a separate PR.

Ah yes, you're completely right, sorry.

Maybe we should introduce a deprecation ahead of 8?

[pbugnion]

Actually, since _MultipleSelection also relies on _make_options, doesn't this PR break support for dict-like objects in widgets that inherit from _MultipleSelection?

For instance, unless I'm missing something, the following is broken:

import ipywidgets.widgets.widget_selection as iws
s2 = iws._MultipleSelection(options={'a': 52, 'b': 62}, value=[62])

This works on master, but fails on your branch. AFAICT, on your branch, _make_options converts options to [('a', 'a'), ('b', 'b')].

Happy to look into this together -- I've found the logic in _MultipleSelection very difficult to follow.

[pbugnion]

Somewhat unexpectedly, for a dictionary d = { "a": 5, "b": 8}, tuple(d) returns the dictionary keys:

>>> d = {"a": 5, "b": 6}
>>> tuple(d)
#=> ("a", "b")

This means that, for someone who's not paid attention to the deprecation warning and still has a dict, their widget won't crash -- it'll just behave really unexpectedly: the dropdown entries will be the same, but the values will just be ignored. I think this is going to be really surprising.

We can either:

1. not care
2. keep the if isinstance(x, Mapping): in _make_options but raise a TypeError if x is a Mapping.

[jasongrout]

keep the if isinstance(x, Mapping): in _make_options but raise a TypeError if it's x is a Mapping.

I'm fine with this, say for one more version.

[jtpio]

Thanks @pbugnion for investigating this further.

Actually, since _MultipleSelection also relies on _make_options, doesn't this PR break support for dict-like objects in widgets that inherit from _MultipleSelection?

It looks like it does yes. We should be able to catch that in the tests though.

In the long run we'll probably want to drop support for mutable types for MultipleSelection widgets too. But it would be better to introduce a DeprecationWarning first.

2. keep the if isinstance(x, Mapping): in _make_options but raise a TypeError if x is a Mapping.

This could be a good compromise in the meantime.

[jasongrout]

Just to be clear here - we are talking about dropping support for mapping types, not mutable types.

[jasongrout]

@jtpio - what is the current status of this PR?

[jtpio]

It was changed back to a "WIP" to check the points raised by @pbugnion above and see if they can be covered in the tests too:

Actually, since _MultipleSelection also relies on _make_options, doesn't this PR break support for dict-like objects in widgets that inherit from _MultipleSelection?

[jtpio]

We can indeed see that with a SelectMultiple widget:

from ipywidgets import SelectMultiple

options = {'a': 'Apples', 'o': 'Oranges', 'p': 'Pears'}

select = SelectMultiple(
    options=options,
    description='Fruits'
)
select

Which gives back the list of keys as values:

We might then want to drop support for Mapping types as _MultipleSelectionoptions too. The DeprecationWarning added in #2130 should also be triggered when using a SelectMultiple widget in 7.5:

[jtpio]

keep the if isinstance(x, Mapping): in _make_options but raise a TypeError if x is a Mapping.

This would at least make the change more visible instead of silently converting the Mapping to a tuple.

[jtpio]

Currently, the options traitlet has type Any(). I wonder if we can change it to a traitlets.List or some such?

We can also follow this approach and be explicit about the types being accepted. For example with Union([Tuple(), Set()]).

Edit: although we might want to keep support for the numpy case: #2679 (comment)

[jtpio]

The last commit implements the ideas mentioned above:

• raise a TypeError for Mapping types, since the DeprecationWarning was already given on other selection widgets
• this prevents implicit casts to a tuple
• numpy arrays are still supported

[jasongrout]

@jtpio - thanks! @pbugnion - do you have time to review the current changes?

[pbugnion]

Great, thank you!

I'll review this in the next couple of days.

[pbugnion]

I added a couple of tests to verify the behaviour when setting .options, clarified a docstring, and added a note on changes in ipywidgets 8.

RTD is failing because of #2768, but the rest looks good. I'm going to merge this.

Thank you!

[jtpio]

Thanks @pbugnion!