Skip to content

Feature/validated tuple#205

Merged
mdickinson merged 32 commits into
masterfrom
feature/validated-tuple
Jan 6, 2015
Merged

Feature/validated tuple#205
mdickinson merged 32 commits into
masterfrom
feature/validated-tuple

Conversation

@itziakos
Copy link
Copy Markdown
Member

@itziakos itziakos commented Oct 6, 2014

This PR adds a new Tuple trait that supports custom validation for the field values in addition to the individual field validation.

>>> from traits.api import Int, HasStrictTraits, ValidatedTuple
>>> 
>>> class MyClass(HasStrictTraits):
...     value_range = ValidatedTuple(Int(0), Int(1), validation=lambda x: x[0] < x[1])
... 
>>>
>>> a = MyClass()
>>> a.value_range
(0, 1)
>>> a.value_range = (2, 1)
Traceback (most recent call last):
  File "<interactive input>", line 1, in <module>
  File "c:\users\itziakos\projects\meta-geophysics\geocanopy\geophysics\util\validated_tuple.py", line 43, in validate
    self.error(object, name, value)
  File "C:\Users\itziakos\Projects\traits\traits\trait_handlers.py", line 172, in error
    value )
TraitError: The 'value_range' trait of a MyClass instance must be a tuple of the form: (an integer (int or long), an integer (int or long)) that passes custom validation, but a value of (2, 1) <type 'tuple'> was specified.
>>> a.value_range = (1, 2)
>>> a.value_range
(1, 2)
>>> 

@itziakos
Copy link
Copy Markdown
Member Author

itziakos commented Oct 6, 2014

Thanks @rkern, This PR is ready for review

@itziakos
Copy link
Copy Markdown
Member Author

itziakos commented Oct 6, 2014

I could probably provide add an optional string in the trait metadata to be used for the TraitError. For example if would be nice is we could get this error

TraitError: The 'value_range' trait of a MyClass instance must be a tuple of the form: (an integer (int or long), an integer (int or long)) that passes custom validation: value_range[0] < value_rane[1], but a value of (2, 1) <type 'tuple'> was specified.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.16%) when pulling 2dc9ad6 on feature/validated-tuple into 1ce982e on master.

Comment thread traits/tests/test_validated_tuple.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we change the name of this keyword argument? validator would be more accurate than validation. Alternatively, how about just valid (to be thought of as "is_valid").

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The convention in Traits seems to be validate for this kind of argument.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Works for me.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

there is a method called validate in TraitType. I have renamed the keyword to fvalidatewhich is the same used for Property traits.

@mdickinson
Copy link
Copy Markdown
Member

We need docs for the new Trait type.

@mdickinson mdickinson self-assigned this Oct 7, 2014
Comment thread traits/trait_types.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Too much whitespace! Please remove the last blank line inside the docstring, to be consistent with the surrounding style.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

removed

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.09%) when pulling 81fe37f on feature/validated-tuple into 1ce982e on master.

@itziakos
Copy link
Copy Markdown
Member Author

@mdickinson, the PR is now updated. I tried to follow the local style, please let me know if I missed anything.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.11%) when pulling 81b3cc5 on feature/validated-tuple into 1ce982e on master.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.11%) when pulling 81b3cc5 on feature/validated-tuple into 1ce982e on master.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.11%) when pulling 8d3138a on feature/validated-tuple into 1ce982e on master.

@itziakos
Copy link
Copy Markdown
Member Author

please delay review. I should add a change log comment

@itziakos
Copy link
Copy Markdown
Member Author

@mdickinson, the PR is ready for another review round

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.24%) when pulling f2ca313 on feature/validated-tuple into 87e0bab on master.

@mdickinson
Copy link
Copy Markdown
Member

@itziakos: I've tweaked a bit. Please let me know if you're happy with the changes in the last 4 commits.

@mdickinson mdickinson assigned itziakos and unassigned mdickinson Jan 5, 2015
@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.24%) when pulling ecef9ec on feature/validated-tuple into 87e0bab on master.

@itziakos
Copy link
Copy Markdown
Member Author

itziakos commented Jan 6, 2015

@mdickinson, changes look ok.

@mdickinson
Copy link
Copy Markdown
Member

@itziakos: Thanks. Merging.

mdickinson added a commit that referenced this pull request Jan 6, 2015
@mdickinson mdickinson merged commit 0ec0cc7 into master Jan 6, 2015
@mdickinson mdickinson deleted the feature/validated-tuple branch January 6, 2015 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants