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

Add DeprecationWarning when using Mapping for options #2130

Merged
merged 7 commits into from Aug 5, 2018

Conversation

Projects
None yet
3 participants
@jtpio
Contributor

jtpio commented Jul 11, 2018

Fixes #1958.

For Selection Widgets, add a DeprecationWarning whenever a Mapping (dictionaries) is used as options.

I'll do a second pass on the documentation to remove all mentions to dictionaries as possible values for options.

TODO

  • Fix Python 2 test
  • Second pass on the documentation

jtpio added some commits Jul 11, 2018

Add DeprecationWarning when using Mapping for options
For Selection Widgets, add a `DeprecationWarning` whenever a `Mapping`
(dictionaries) is used as options.
Use simplefilter('always') for testing the warning
* Use `warnings.simplefilter('always')` for DeprecationWarning
* More specific test on warning message
@jasongrout

This comment has been minimized.

Member

jasongrout commented Jul 16, 2018

Thank you!

Can you remove the model_id changes in the PR for the notebook? That's churn that breaks the saved widget state in the notebook. We'll run all the notebooks before release and commit one big update with the updated widget state.

(to be clear, please keep the source text changes in the notebook, and the one place where we have printed output that would change).

@jtpio

This comment has been minimized.

Contributor

jtpio commented Jul 17, 2018

Sure thing! And it also makes the diff cleaner.

@jasongrout jasongrout added this to the 7.3 milestone Jul 17, 2018

@jasongrout

This comment has been minimized.

Member

jasongrout commented Jul 17, 2018

Adding to 7.3 - let's see if we can get this in...

@jasongrout

This comment has been minimized.

Member

jasongrout commented Jul 17, 2018

Looks like there is a test failure. We'll try to get this in first thing for 7.4.

@jasongrout jasongrout modified the milestones: 7.3, Minor release Jul 17, 2018

@jtpio

This comment has been minimized.

Contributor

jtpio commented Jul 17, 2018

The failure is related to the new TestDropdown test and seems to be only happening for Python 2.7.

Not sure what it is yet as the tests pass locally with Python 2.7.14

Explicitly clear the warning registry for tests
As an attempt to fix Python 2 behavior.
@jtpio

This comment has been minimized.

Contributor

jtpio commented Jul 26, 2018

Tests are now green after the last commit.

There seems to be a different behavior between Python 2.7 and Python 3 when it comes to testing warnings (even though the documentation looks identical between the two versions: https://docs.python.org/3.7/library/warnings.html#testing-warnings).

Python 2 requires to manually clear the warning registry if we want to be able to catch them:

import inspect
module = inspect.getmodule(Dropdown)
getattr(module, '__warningregistry__', {}).clear()

This has also been mentioned in other project testing for warning being triggered, for example with Matplotlib: matplotlib/matplotlib@0017d3e

@SylvainCorlay

This comment has been minimized.

Member

SylvainCorlay commented Aug 3, 2018

@jtpio these seems to be a conflict. Could you please rebase your PR on master?

@jasongrout

This comment has been minimized.

Member

jasongrout commented Aug 3, 2018

@SylvainCorlay - We should try to get this in for 7.4 as well.

@jtpio

This comment has been minimized.

Contributor

jtpio commented Aug 3, 2018

test_widget_selection.py was added in both master and this branch.

Should be good now 👍

@jasongrout

This comment has been minimized.

Member

jasongrout commented Aug 3, 2018

That's an awesome problem, that we're tripping over ourselves to add more testing :).

@SylvainCorlay SylvainCorlay merged commit d848427 into jupyter-widgets:master Aug 5, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jtpio jtpio deleted the jtpio:mapping-deprecation-warning branch Aug 5, 2018

@SylvainCorlay SylvainCorlay modified the milestones: Minor release, 7.4 Aug 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment