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

Using the early validation hook for IntSlider and FloatSlider #8142

Merged
merged 5 commits into from Mar 29, 2015

Conversation

SylvainCorlay
Copy link
Member

@jdfreder @minrk @jasongrout indeed, during initial setting I must prevent the cross validation.

@SylvainCorlay SylvainCorlay force-pushed the slider_validation branch 2 times, most recently from e1061c0 to 1e63456 Compare March 25, 2015 19:11
@minrk
Copy link
Member

minrk commented Mar 26, 2015

Can you verify that passing conflicting values on init still fails by adding a few tests?

slider = IntSlider(min=1, max=0, value=-1)

@minrk minrk added this to the 4.0 milestone Mar 26, 2015
@SylvainCorlay
Copy link
Member Author

Currently, validation does not occur on init. We cannot use cross validation functions like _*_validate on init since they require other trait values that may not yet be initialized...
We could actually have an explicit check in the constructor which may throw a TraitError.

@minrk
Copy link
Member

minrk commented Mar 26, 2015

Okay, then I think this one should wait until you add delayed validation to the hold_notifications context (or however it makes the most sense to implement that)

@SylvainCorlay SylvainCorlay force-pushed the slider_validation branch 3 times, most recently from 612af5d to 9e8b1f4 Compare March 27, 2015 18:13
@SylvainCorlay
Copy link
Member Author

@minrk Here is a version of the context manager holding validation. Let me know what you think.

@minrk
Copy link
Member

minrk commented Mar 27, 2015

I think it looks sensible, but it would appear to not be working just yet. Lots of test failures that look like trait values aren't being set.

@SylvainCorlay
Copy link
Member Author

This is ready for further review.

@@ -215,7 +215,7 @@ def excepthook(self, etype, evalue, tb):
return crashhandler.crash_handler_lite(etype, evalue, tb)

def _ipython_dir_changed(self, name, old, new):
if old is not None:
if old is not None and old is not Undefined:
Copy link
Member

Choose a reason for hiding this comment

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

Why can old be Undefined, now?

Copy link
Member Author

Choose a reason for hiding this comment

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

In traitlets.py Line 449, I changed the value of old from None to Undefined in case of key error.

The reason for that change, is that to roll back in case of bulk rejection, I need to distinguish between unset attributes and attributes set to None.

Copy link
Member

Choose a reason for hiding this comment

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

ok, makes sense. There may be other cases affected by this. We'll have to keep an eye out.

@SylvainCorlay
Copy link
Member Author

Added a test.

minrk added a commit that referenced this pull request Mar 29, 2015
Using the early validation hook for IntSlider and FloatSlider
@minrk minrk merged commit e99202c into ipython:master Mar 29, 2015
@SylvainCorlay SylvainCorlay deleted the slider_validation branch March 29, 2015 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants