Update FileSelector parameters when setting path #476
Conversation
philippjfr
changed the title
philippjfr
force-pushed
the
fileselector_path_update
branch
4 times, most recently
from
01aff6b
to
179a885
Compare
|
My main question is whether this mechanism might be better used to generate warnings instead of errors? |
philippjfr
force-pushed
the
fileselector_path_update
branch
from
f03da72
to
381b74c
Compare
philippjfr
force-pushed
the
fileselector_path_update
branch
from
381b74c
to
a0387b5
Compare
philippjfr
changed the title
|
Reverted any changes to validation on setting but still factored out separate validation methods for different attributes which should make it straightforward to readd the validation later. |
|
Makes sense. Ready for review? |
|
Yes, note this PR also continues some style and exception cleanup. Should have kept that separate sorry. |
| @@ -750,46 +750,37 @@ class Number(Dynamic): | |||
|
|
|||
| """ | |||
|
|
|||
| __slots__ = ['bounds','_softbounds','inclusive_bounds','set_hook', 'step'] | |||
| __slots__ = ['bounds', 'softbounds', 'inclusive_bounds', 'set_hook', 'step'] | |||
There was a problem hiding this comment.
Getting rid of the underscore makes sense as the properties didn't really do anything, but couldn't this have backward compatibility implications?
There was a problem hiding this comment.
I suppose stateful things like pickling shouldn't touch slots directly and the API is the same as it was when properties were used. It should be ok but I am always wary when messing with slots in param.
|
|
||
| def _validate(self, val): |
There was a problem hiding this comment.
Good to see the types of validation performed laid out like this!
|
|
||
| return val | ||
|
|
||
| def _validate_bounds(self, val, bounds, inclusive_bounds): |
There was a problem hiding this comment.
I like this: more general than _checkBounds and makes the input explicit using arguments instead of self.
There was a problem hiding this comment.
Yeah, this is also in preparation to supporting validate methods on the class itself which could be useful for composite parameters.
|
|
||
| def _validate(self, val): | ||
| assert len(val) == len(self.attribs),"Compound parameter '%s' got the wrong number of values (needed %d, but got %d)." % (self.name,len(self.attribs),len(val)) |
There was a problem hiding this comment.
Guess I've never seen this assert raise an exception so I guess it is safe to remove...
There was a problem hiding this comment.
Not removed, just moved to _validate_attribs as a proper ValueError
| raise ValueError( | ||
| "Parameter '%s' must be a subclass of %s, not '%s'" % | ||
| (val.__name__, class_name, val.__class__.__name__)) | ||
| "%s parameter %r must be a subclass of %s, not %r." % |
There was a problem hiding this comment.
I like this improvement to the exception message...
|
Reviewing with PR was a little tricky with all the formatting changes but every code change looks like an improvement to me. Happy to see this merged! |
Adds an
_on_setmethod to all Parameters, this allows Parameters to trigger actions when one of its attributes changes. This is used to update the list of valid paths when(Multi)FileSelector.pathis set.Fixes #412