fix int_validator #1692
fix int_validator #1692
Conversation
|
fixes #1678 |
| @@ -16,6 +16,9 @@ | |||
| import ckan.new_tests.lib.navl.test_validators as t | |||
|
|
|||
|
|
|||
| eq = nose.tools.eq_ | |||
There was a problem hiding this comment.
I guess we agreed on using assert_equals and assert_not_equals in #1484
There was a problem hiding this comment.
Oh? I only saw assert_equals in one place in the new tests. Happy to change it.
|
@vitorbaptista can you have a look at this? |
|
@amercader sure! @wardi Could you change your files to use single quotes? While you're at it, it would be good to change the few other methods in the files you touched to use single quotes as well, so we're a bit closer to our coding standards. |
| @@ -444,5 +447,55 @@ def call_validator(*args, **kwargs): | |||
| *args, **kwargs) | |||
| call_validator(key, data, errors, context={'model': mock_model}) | |||
|
|
|||
| def test_int_validator_idempotent(self): | |||
| import ckan.logic.validators as validators | |||
There was a problem hiding this comment.
I see that we use this pattern of importing what the tests need in the test itself everywhere in this file, but I haven't seen it used in other places. Do you know if it's something we should start doing? If not, I would prefer to move those imports to the beginning of the file, following the relative coding standards
| 1 + 0j, # int(complex) fails, so expect the same | ||
| ] | ||
| with warnings.catch_warnings(): | ||
| warnings.filterwarnings("ignore", category=DeprecationWarning) |
There was a problem hiding this comment.
Although I guess this deprecation warning isn't too bad, I still feel a bit nervous ignoring it.
Using divmod() on complex numbers was removed in Python 3. I know we won't be moving to Python 3 any time soon, but maybe it's simple enough to test if the number is complex and don't run divmod() in this case?
If you can't find an easy way to do so, I guess I'd rather see the deprecation message than ignore it (although I can be convinced otherwise).
| inputs or non-whole values | ||
| ''' | ||
| if value is None: | ||
| return None |
There was a problem hiding this comment.
I haven't tested it, but is this a new behavior? Just by looking at the diff, it seems that we used to raise Invalid if called with None.
Just making sure it was intended.
There was a problem hiding this comment.
Yes, this is new behaviour.
The current implementation is not idempotent. If you call it once passing in an empty string it will convert that to None, but calling a second time will raise an Invalid exception. The same is true if you pass a long value as a string: it fails on the second call. This PR fixes those cases and some others.
I believe that all validators should be idempotent: it shouldn't matter if you call them once or twenty times in a row, you should always get the same result.
There was a problem hiding this comment.
This made me realize that I had the wrong definition of idempotent. I thought it was simply that a function doesn't have side effects, and if I call it n times with the same parameters, it'll give me the same result. Basically, f(x) == f(x) == f(x), but checking Wikipedia tells me that it's actually f(f(f(x))) == f(x). Thanks!
int_validatoris currently broken in at least three cases:Tests to come