Adding support for Decimal type (Decimal module) #332

Amedeo91 · 2017-08-15T17:19:46Z

This PR has the goal to provide support to validate decimal data type.

This way allows checking whether the number in the request is parsable by the decimal module. Note: this is to provide support for the new decimal type in mongodb.

funkyfuture · 2017-08-15T17:44:46Z

i'm not sure whether that makes sense. in opposite to other possible constraints, this test doesn't relate to an actual Python type. it tests whether a value can be used to initialize a decimal.Decimal object. that could also by achieved with oneof, type and regex.

in the not-so-fundamental-points category:

there is no docs extension
please keep methods sorted!
version constants should be imported from the platform module

now, some more fundamental stuff again:

the code shouldn't be available on platforms without the decimal module

i tend to recommend you to use it custom validators.

Amedeo91 · 2017-08-15T18:00:16Z

Hi!

decimal module is native in python since version 2.4, therefore all python version has it.

Regarding your question, I understand your point, but it is used a lot to provide support for prices and everywhere you want to keep the decimal part without approximation.
Therefore, for my point of view, this validation should be native in the json validator. For the documentation, I will do it after I am able to make the CI passing.

Regards,
Amedeo

Amedeo91 · 2017-08-15T19:31:09Z

@nicolaiarocci I did the PR to start to add the decimal data type to Cerberus in order to have it available in pyeve/eve as well.

Regards,
Amedeo

Note: python 3.5 is failing for everybody for some problem in the documentation which i was not able to understand.

funkyfuture · 2017-08-15T19:37:20Z

it'd make sense to test for decimal.Decimal after a value has been coerced. the way it is implemented now, it's just confusing because cerberus will tell you that something is of a typte that it isn't. this is everything else than intuitive with regards to the current semantics.

Amedeo91 · 2017-08-15T19:43:50Z

I do not really understand what it is not clear for you: how would you implement this behavior?

For my point of view, the Decimal module can be considered as a different way to represent numbers.
Therefore, it makes total sense for me to validate as a data type. But, if you have a different proposal, please let me know! :)

Regards,
Amedeo

funkyfuture · 2017-08-15T19:54:19Z

i have no doubt that decimal.Decimal representations are a valid representations of numbers.

it makes total sense for me to validate as a data type.

yes, but then do exactly that. validate the type of a symbol, not what a value could be coerced to. if you want to use it as Decimal, you'd have to convert it in the normalization phase anyway.

new decimal type in mongodb

just for my understanding, does the Python client have a strict concorance with the Python type of the same name?

Amedeo91 · 2017-08-15T20:01:46Z

Just for your understanding of the issue, look at this: pyeve/eve#1045

App which uses mongodb needs to format the data type accordingly.

"yes, but then do exactly that. validate the type of a symbol, not what a value could be coerced to. if you want to use it as Decimal, you'd have to convert it in the normalization phase anyway." --> not able to follow you of how to do it: can you give me an example?

funkyfuture · 2017-08-16T19:59:34Z

not able to follow you of how to do it: can you give me an example?

the docs have examples on coercion.

from what i understand here, the MongoDB-client uses its own type.

Amedeo91 · 2017-08-16T23:11:01Z

decimal128 is a sub-class of the decimal module. The only difference is that it accepts only string as an input of the module, but I do not think we should enforce it in the json validator as it is a specificity of mongoDb.

For coerce, unfortunately, I need to use the type field in order to be able to implement it in Eve. For this point, I would prefer to wait for @nicolaiarocci feedback if you are ok with that so that we design together how to add the support in both Cerberus and Eve. Thank you for your feedback

Regards,
Amedeo

nicolaiarocci · 2017-08-17T12:58:24Z

Since decimal is supported by Python, I think that supporting it as a base type is a good idea.

funkyfuture · 2017-08-17T16:33:54Z

Since decimal is supported by Python, I think that supporting it as a base type is a good idea.

there is no dissent in this regard.

though it becomes critical when more and more types from the standard library are supported and the footprint (import execution time, memory) of an application that uses cerberus increases with each imported module. so far only builtin types and some from datetime and collections.abc are covered.

Special cases aren't special enough to break the rules.

my critique is still that the implementation makes no sense considering how Cerberus' type validations operate. it's understandable that the following inconsistency would alienate users:

>>> schema = {'a_bool': {'type': 'boolean'},
...	      'a_date': {'type': 'date'},
...           'a_decimal': {'type': 'decimal'},
...           'another_decimal': {'type': 'decimal'}}
...
>>> document = {'a_bool': True,
...             'a_date': datetime.date(1945, 5, 9),
...             'a_decimal': 10,
...             'another_decimal': '100'}
...
>>> validator(document, schema)
True
>>> isinstance(document['a_bool'], bool)
True
>>> isinstance(document['a_date'], datetime.date)
True
>>> isinstance(document['a_decimal'], decimal.Decimal)
False
>>> isinstance(document['another_decimal'], decimal.Decimal)
False

In the face of ambiguity, refuse the temptation to guess.

maybe a bad allegory, but who would trust a lying validator?

i'd appreciate feedback regarding the particular points i raise by anyone from the user community.

Amedeo91 · 2017-08-17T16:42:31Z

I understand youf point. What if we automatically convert the input to decimal in case it is parsable? After all, we are talking about number: I think it makes sense! Let me know your opinion. :)

Regards,
Amedeo

funkyfuture · 2017-08-17T16:45:50Z

What if we automatically convert the input to decimal in case it is parsable?

that is why i pointed to the value coercion. once a value is coerced it will pass a proper type test.

Amedeo91 · 2017-08-17T17:49:39Z

I did not really get that until now!
Therefore, you ask I will just check the instance type and eventually coerce it.

Ok, I will update

funkyfuture · 2017-08-17T18:18:01Z

normalization is applied before validation. as documented.

Amedeo91 · 2017-08-17T19:58:10Z

Ok, I did the update in order to make it work as you requested.

Regards,
Amedeo

funkyfuture · 2017-08-18T20:34:30Z

propably the cleanest and safest way is to put this in the platform module:

DECIMAL_TYPES = (decimal.Decimal,)
try:
    from bson import Decimal128
except ImportError:
    pass
else:
    DECIMAL_TYPES += Decimal128

and then use that constant as second argument for isinstance.

funkyfuture · 2017-08-18T20:35:31Z

would it suffice to test that particular type in Eve as it already has that dependency?

funkyfuture · 2017-08-18T20:36:56Z

cerberus/tests/test_validation.py

@@ -393,6 +404,22 @@ def test_integer():
    assert_success({'an_integer': 50})


+def test_decimal():
+    # Module decimal does not accept integer or float for version 2.6
+    if int(sys.version_info[0]) > 2 and int(sys.version_info[1]) > 6:


there's the PYTHON_VERSION constant in platform for that.

funkyfuture · 2017-08-18T20:37:24Z

cerberus/tests/test_validation.py

@@ -1270,8 +1297,20 @@ def test_oneof_schema():


 def test_nested_oneof_type():
-    schema = {'nested_oneof_type':
-              {'valueschema': {'oneof_type': ['string', 'integer']}}}
+    schema = \


i appreciate the other reformattings, but this is unnecessary.

put it back (I change it as I had a problem with the flake8 validation...but now it is working fine)

funkyfuture · 2017-08-18T20:37:44Z

cerberus/validator.py

@@ -11,6 +11,7 @@
 from __future__ import absolute_import

 from ast import literal_eval
+import decimal


please keep imports sorted.

updating! Thanks

funkyfuture · 2017-08-18T20:38:24Z

cerberus/validator.py

@@ -1240,6 +1241,14 @@ def _validate_type_integer(self, value):
        if isinstance(value, _int_types):
            return True

+    def _validate_type_decimal(self, value):
+        if isinstance(value, decimal.Decimal):


i'd prefer this.

funkyfuture · 2017-08-18T20:39:28Z

docs/validation-rules.rst

+     - :class:`py2:decimal.Decimal()`
+     - :class:`py3:decimal.Decimal() or bson.decimal128.Decimal128()`
+
+.. versionchanged:: 1.1


these comments are below.

Amedeo91 · 2017-08-18T22:02:45Z

@funkyfuture I did the updated following your code review! :)

Thanks,
Amedeo

funkyfuture

sorry if some remarks seem cynical, they are.

i'm under the impression you don't care much a clean code or take hints as impulse for own investigations. when contributing to a new project it is advisable to study its code and get familar with it. also, sleeping a night over one's own changes and then self-reviewing them makes a difference in the result's quality.

funkyfuture · 2017-08-19T09:51:53Z

cerberus/validator.py

@@ -1240,6 +1249,11 @@ def _validate_type_integer(self, value):
        if isinstance(value, _int_types):
            return True

+    def _validate_type_decimal(self, value):


please keep methods sorted. maintainability is must.

They are already not sorted!! But ok, I will move mine

funkyfuture · 2017-08-19T09:55:41Z

cerberus/validator.py

@@ -1240,6 +1249,11 @@ def _validate_type_integer(self, value):
        if isinstance(value, _int_types):
            return True

+    def _validate_type_decimal(self, value):
+        for decimal_type in DECIMAL_TYPES:


please re-read the last sentence here. it's always advisable to study the function's documentation one uses. and the docs of the library one uses. ;-)

oops! Sorry, I have it in my local! I will push now

funkyfuture · 2017-08-19T09:57:17Z

docs/validation-rules.rst

@@ -691,6 +695,9 @@ A list of types can be used to allow different values:
    fields. This allows to safely assume that field type is correct when other
    (standard or custom) rules are invoked.

+.. versionchanged:: 1.1
+  From this version, a new data type to support decimal module has been introduced.


indentation is off. a formulation and formatting like for other types seems appropriate. 1.1 is already released.

Sorry, I am updating the version and indentation :)

funkyfuture · 2017-08-19T09:58:09Z

docs/validation-rules.rst

@@ -648,6 +648,10 @@ Data type allowed for the key value. Can be one of the following names:
   * - ``string``
     - :func:`py2:basestring`
     - :class:`py3:str`
+   * - ``decimal``


add '(if available)' as annotation for the latter Python type.

The type is always available as decimal module. Only decimal128 might be not available as part of bson module (which exist for all python version)

funkyfuture · 2017-08-19T10:00:45Z

cerberus/tests/conftest.py

+    'a_decimal': {
+        'type': 'decimal',
+    },
+    'a_decimal_coercing': {


this schema is only used for validation so far. it stems back from a time when Cerberus was far less complex. putting more complex stuff here has a chance to break a lot of tests.
there's a test module for normalizations you propably have noticed when you studied the code.

ok, I will find another place for it.

funkyfuture · 2017-08-19T10:02:32Z

cerberus/tests/test_validation.py

-import re
-import sys
+
+from bson import decimal128


to repeat myself: would it suffice to test that in Eve? i'd rather avoid extra dependencies, even for testing.

Sorry, I did not get this point before (too many comments): since it is a functionality offered by Cerberus, I think it should be tested within it. But I let you take the decision over it

Amedeo91 · 2017-08-19T10:28:10Z

@funkyfuture I do not really see why you are cynical in the first place.....
I am trying to follow your hints when I can and when I have time to do it

nicolaiarocci · 2017-08-20T05:39:49Z

Decimal type support is going to be implemented in Eve's own custom validator instead.

Amedeo91 force-pushed the my_changes branch 3 times, most recently from 27b11c4 to 7f90311 Compare August 15, 2017 19:27

Amedeo91 force-pushed the my_changes branch from 7f90311 to 13b6394 Compare August 17, 2017 19:54

funkyfuture requested changes Aug 18, 2017

View reviewed changes

Amedeo91 force-pushed the my_changes branch 4 times, most recently from 761580a to 9e0829a Compare August 18, 2017 21:58

funkyfuture requested changes Aug 19, 2017

View reviewed changes

Amedeo91 force-pushed the my_changes branch 2 times, most recently from eb280c5 to 6138a9f Compare August 19, 2017 10:43

Adding support for Decimal type (Decimal module)

c7e39a8

Amedeo91 force-pushed the my_changes branch from 6138a9f to c7e39a8 Compare August 19, 2017 12:04

nicolaiarocci closed this Aug 20, 2017

funkyfuture mentioned this pull request Aug 20, 2017

Refactors type validation to a descriptive paradigm #335

Closed

Adding support for Decimal type (Decimal module) #332

Adding support for Decimal type (Decimal module) #332

Conversation

Amedeo91 commented Aug 15, 2017

funkyfuture commented Aug 15, 2017 • edited

Amedeo91 commented Aug 15, 2017

Amedeo91 commented Aug 15, 2017

funkyfuture commented Aug 15, 2017

Amedeo91 commented Aug 15, 2017

funkyfuture commented Aug 15, 2017

Amedeo91 commented Aug 15, 2017

funkyfuture commented Aug 16, 2017

Amedeo91 commented Aug 16, 2017

nicolaiarocci commented Aug 17, 2017

funkyfuture commented Aug 17, 2017 • edited

Amedeo91 commented Aug 17, 2017

funkyfuture commented Aug 17, 2017

Amedeo91 commented Aug 17, 2017 • edited

funkyfuture commented Aug 17, 2017 via email

Amedeo91 commented Aug 17, 2017

funkyfuture commented Aug 18, 2017

funkyfuture commented Aug 18, 2017

Choose a reason for hiding this comment

Amedeo91 Aug 18, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Amedeo91 commented Aug 18, 2017

funkyfuture left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Amedeo91 Aug 19, 2017 • edited

Choose a reason for hiding this comment

Amedeo91 commented Aug 19, 2017

nicolaiarocci commented Aug 20, 2017

funkyfuture commented Aug 15, 2017 •

edited

funkyfuture commented Aug 17, 2017 •

edited

Amedeo91 commented Aug 17, 2017 •

edited

Amedeo91 Aug 18, 2017 •

edited

Amedeo91 Aug 19, 2017 •

edited