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

Allow Mappings to be given as the options in a selection widget again. #3557

Merged
merged 5 commits into from
Aug 30, 2022

Conversation

jasongrout
Copy link
Member

@jasongrout jasongrout commented Aug 20, 2022

We deprecated this long ago when dicts were not ordered (#2130, #1958, #2679), then removed it in ipywidgets 8.0. However, now dicts are ordered by default, so perhaps we can put it back in and keep people's code from breaking.

I want to place a high value on easy ways to support backwards compatibility, as many people would rather just not change their code if they can help it.

However, if we are going to stick with this decision in 8.0, #3556 makes the error more helpful. #3556 and this PR are mutually exclusive - at most one should be merged.

We deprecated this long ago when dicts were not ordered (jupyter-widgets#2130, jupyter-widgets#1958), then removed it in ipywidgets 8.0. However, now dicts are ordered by default, so perhaps we can put it back in and keep people's code from breaking.
@github-actions
Copy link

Binder 👈 Launch a binder notebook on branch jasongrout/ipywidgets/optiondicts

@jasongrout
Copy link
Member Author

The expected tests fail. If we decide to do this PR, I can fix the tests.

Copy link
Member

@vidartf vidartf left a comment

Choose a reason for hiding this comment

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

There was a general consensus in the dev meeting today that it was probably best to add back the support now that dicts are ordered by default, but to not put any use/mention of this pattern in the docs. @maartenbreddels also argued for not mentioning it in the docstring either, but instead just leave a comment in the code that this support is here for backwards compatibility reasons.

@vidartf
Copy link
Member

vidartf commented Aug 23, 2022

Tests will then need fixing.

This essentially does a partial reversion of 9db179b and 28877f9 (jupyter-widgets#2679)
@jasongrout jasongrout added this to the 8.0 milestone Aug 26, 2022
@jasongrout jasongrout requested a review from vidartf August 26, 2022 11:35
@jasongrout
Copy link
Member Author

@vidartf - rerequesting review, since tests pass now.

Also pinging @jtpio and @pbugnion, who helped remove the dict option and deprecated it in 7.x (though it appears the deprecation warning was never really seen by users?)

@jtpio
Copy link
Member

jtpio commented Aug 26, 2022

though it appears the deprecation warning was never really seen by users?

I believe the warning was shown as suggested by the second screenshot in #2679 (comment)?

@vidartf
Copy link
Member

vidartf commented Aug 26, 2022

I'll leave off the merge for now to leave some time for others to comment, but looks ok to me!

@jasongrout
Copy link
Member Author

jasongrout commented Aug 26, 2022

I believe the warning was shown as suggested by the second screenshot in #2679 (comment)?

Only if you reset the DeprecationWarning filter to always show deprecation warnings (or ran Python with the appropriate switches to always show deprecation warnings, etc.), as shown in that screenshot. I imagine a vanishingly small fraction of our users did this?

I guess what I realized last night was that Python has had a pretty bad story about surfacing deprecation warnings to users for a lot of the 3.x series, and as mentioned in the threads linked in #3569, it is tricky to get the appropriate stacklevel so that even the much better automatic surfacing of warnings in Python 3.7+ works.

I stared at this for a while before I realized we had check_widget and check_widgets. This rename makes the distinction between these functions more clear.
@jtpio
Copy link
Member

jtpio commented Aug 26, 2022

Only if you reset the DeprecationWarning filter to always show deprecation warnings (or ran Python with the appropriate switches to always show deprecation warnings, etc.), as shown in that screenshot. I imagine a vanishingly small fraction of our users did this?

Oh right, then yes probably not many users were able to see it.

In that case yes it makes sense to revert it since most users probably didn't get the notice 👍

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.

3 participants