Fix set validator #52
Conversation
A new function stringify_set only stringifies n elements of a given set.
vladiate/test/test_validators.py
Outdated
| @@ -85,6 +85,7 @@ def test_set_validator_works(field_set, field): | |||
| ([], 'bar'), | |||
| (['foo'], 'bar'), | |||
| (['foo', 'bar'], 'baz'), | |||
| ([str(x) for x in range(200)], "notanumber"), | |||
There was a problem hiding this comment.
Would you be able to add some actual tests for stringify_set instead of this?
There was a problem hiding this comment.
I just found out that stringify_set may not be deterministic since the output is dependent on the order of elements that are generated by the set iterator.
For Python 2.7.12:
$ python2
>>> {'A', 'B', 'C', 'D', 'E', 'F'}
set(['A', 'C', 'B', 'E', 'D', 'F'])
For Python 3.5.2: It can be different every time the python interpreter is started!
$ python3
>>> {'A', 'B', 'C', 'D', 'E', 'F'}
{'D', 'B', 'E', 'F', 'A', 'C'}
>>> exit()
$ python3
>>> {'A', 'B', 'C', 'D', 'E', 'F'}
{'C', 'F', 'E', 'A', 'B', 'D'}
>>> exit()
This makes it quite difficult for proper testing. Should we sort the elements of the set first? But then its quite inperformant if the set is large (see #50)
vladiate/validators.py
Outdated
| def stringify_set(a_set, max_len): | ||
| ''' Stringify `max_len` elements of `a_set` and count the remainings ''' | ||
| # Don't convert `a_set` to a list for performance reasons | ||
| text = "[{}]".format(", ".join( |
There was a problem hiding this comment.
Small tweak, could you change this to "[{}]" to "{{{}}}" so that this prints the set in braces instead of brackets and is consistent with the way it was before?
There was a problem hiding this comment.
Sure, that makes sense.
Fix stringify_set: Sort the elements before displaying for small sets
|
I fixed the issues you mentioned in the review. |
vladiate/validators.py
Outdated
| @@ -208,3 +210,26 @@ def validate(self, field, row={}): | |||
| @property | |||
| def bad(self): | |||
| pass | |||
|
|
|||
|
|
|||
| def stringify_set(a_set, max_len, max_sort_size=8192): | |||
There was a problem hiding this comment.
Could we rename this to _stringify_set to indicate that it is not part of the public API?
vladiate/validators.py
Outdated
| raise ValueError("max_len must be non-negative: {}".format(max_len)) | ||
| if max_sort_size < 0: | ||
| raise ValueError( | ||
| "max_sort_size must be non-negative: {}".format(max_sort_size)) |
There was a problem hiding this comment.
I appreciate these checks but since this is just a utility function for us and doesn't take user input, I think they're unnecessary. We can just drop d355def entirely.
|
Thanks @syxolk! |
Fix the issue with
SetValidatorin #50: Thevalid_setis no longer stringified as a whole instead only the first 100 elements are shown.Some minor design decisions applied here: