Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature #207

Merged
merged 2 commits into from
Sep 29, 2016
Merged

Feature #207

merged 2 commits into from
Sep 29, 2016

Conversation

nareshnootoo
Copy link
Contributor

Varify the maximum number of digits that are present in the number(Precision),
and the maximum number of decimal places(Scale)

:raises Invalid: If the value does not match the provided Precision and Scale.

>>> s = Schema(Number(precision=6, scale=2))

resource : http://stackoverflow.com/questions/3018758/determine-precision-and-scale-of-particular-number-in-python
"""

def __init__(self, precision=None, scale=None, msg=None, max_digits=14):
Copy link
Collaborator

Choose a reason for hiding this comment

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

make max_digit constant

@@ -557,3 +557,31 @@ def fn(arg):
return "hello"

assert_raises(Invalid, fn, 1)

Copy link
Collaborator

@tusharmakkar08 tusharmakkar08 Sep 21, 2016

Choose a reason for hiding this comment

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

Add following test cases:

  • scale is None
  • precision is None
  • Both are None

frac_digits /= 10
scale = int(math.log10(frac_digits))

if magnitude + scale > self.precision or scale > self.scale :
Copy link
Collaborator

Choose a reason for hiding this comment

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

Make 2 separate Invalid conditions

scale = int(math.log10(frac_digits))

if magnitude + scale > self.precision or scale > self.scale :
raise Invalid(self.msg or "Precision must be smaller than %s, and Scale must be smaller than %s"%(self.precision, self.scale))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will break when self.precision is None or self.scale is None

@nareshnootoo
Copy link
Contributor Author

@tusharmakkar08 I have made some changes as per your suggestion please review

Copy link
Collaborator

@tusharmakkar08 tusharmakkar08 left a comment

Choose a reason for hiding this comment

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

@nareshnootoo I have added few more comments. Please have a look into it.


def test_number_validation_with_valid_number():
""" test with Number with valid precision and scale"""
schema = Schema({"number" : Number(precision=6, scale=2)})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this yield error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In which scenario?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean this scenario should give error and the correct number should be : 1234.00

Copy link
Contributor Author

@nareshnootoo nareshnootoo Sep 23, 2016

Choose a reason for hiding this comment

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

I think we shouldn't be so hard, as 1234 is a valid number, however we can return it with decimal places, what do you think? As here the intension was to limit the scale up to 2.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But the point of having a separate Number validator is to tackle these kind of issues only. If we want 1234, directly to be accepted then instead of using Number we can use int which is already supported by voluptuous.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sense :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But in python if I declare a variable as a=1.000 it always give me 1.0
same in case of PHP it return me as 1
also in general 1.00 == 1

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can try some different implementation. Something on the lines of using Decimal and string conversions.


class Number(object):
"""
Varify the maximum number of digits that are present in the number(Precision),
Copy link
Collaborator

Choose a reason for hiding this comment

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

*Verify

def __repr__(self):
return ('Number(precision=%s, scale=%s, msg=%s)' % (self.precision, self.scale, self.msg))

def get_precision_scale(self, number):
Copy link
Collaborator

Choose a reason for hiding this comment

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

private function

:param number:
:return: tuple(precision, scale)
"""
max_digits = 14
Copy link
Collaborator

Choose a reason for hiding this comment

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

Better make it default argument in the class.

@coveralls
Copy link

coveralls commented Sep 26, 2016

Coverage Status

Changes Unknown when pulling c9de670 on nareshnootoo:feature into * on alecthomas:master*.

@coveralls
Copy link

coveralls commented Sep 26, 2016

Coverage Status

Changes Unknown when pulling fbf40b3 on nareshnootoo:feature into * on alecthomas:master*.

@tusharmakkar08
Copy link
Collaborator

Hey @nareshnootoo,

Travis build is failing. Please update the PR.

Thanks

@coveralls
Copy link

coveralls commented Sep 27, 2016

Coverage Status

Changes Unknown when pulling 4327cc8 on nareshnootoo:feature into * on alecthomas:master*.

@nareshnootoo
Copy link
Contributor Author

Hi @tusharmakkar08 I have implemented the Decimal and string conversions. Please review

Copy link
Collaborator

@tusharmakkar08 tusharmakkar08 left a comment

Choose a reason for hiding this comment

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

Please resolve all the comments and squash your commits. Let's get it merged asap 🍰 👍

else:
assert False, "Did not raise Invalid for String"

def test_number_validation_with_invalid_precision_scale():
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a test case with only precision and no scale.

:raises Invalid: If the value does not match the provided Precision and Scale.

>>> schema = Schema({"number" : Number(precision=6, scale=2)})
>>> out_ = schema({"number": '1234.01'})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add an assert for doctests here as well.

def __repr__(self):
return ('Number(precision=%s, scale=%s, msg=%s)' % (self.precision, self.scale, self.msg))

def __get_precision_scale(self, number):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's make __get_precision_scale internal instead of _get_precision_scale. Refer http://stackoverflow.com/a/6930223/1799475

:param number:
:return: tuple(precision, scale, decimal_number)
"""
from decimal import Decimal
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move this import up.


try:
decimal_num = Decimal(number)
except:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please avoid having generic Exception catching.

@@ -806,7 +806,7 @@ class Number(object):

>>> schema = Schema(Number(precision=6, scale=2))
>>> schema('1234.01')
1234.01
Decimal('1234.01')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Assert here

@coveralls
Copy link

coveralls commented Sep 27, 2016

Coverage Status

Changes Unknown when pulling e6b1b31 on nareshnootoo:feature into * on alecthomas:master*.

@nareshnootoo
Copy link
Contributor Author

Hi @tusharmakkar08 I have fixed the following:

  1. Moved Decimal import up
  2. Added InvalidOperation exception instead of generic
  3. Made __get_precision_scale to internal as _get_precision_scale
  4. Added assert for doctests
  5. Added case with only invalid precision and no scale

Copy link
Collaborator

@tusharmakkar08 tusharmakkar08 left a comment

Choose a reason for hiding this comment

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

Please squash the commits after resolving the comments.


>>> schema = Schema(Number(precision=6, scale=2))
>>> schema('1234.01')
Decimal('1234.01')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add assert here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we do something about Decimal output ? (Maybe have string output). @alecthomas What do you think about this?

Copy link
Contributor Author

@nareshnootoo nareshnootoo Sep 27, 2016

Choose a reason for hiding this comment

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

@tusharmakkar08 Yes we can have string output, but user can use processed Decimal, if he want.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@nareshnootoo Let's add a flag which tells what to yield, where default being the string since we are giving input as string only.

assert_equal(float(out_.get("number")), 123456789012.00)

def test_number_when_precision_none_n_invalid_scale():
""" test with Number with no precision and invalid scale"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's difficult to keep track of all the function names in tests since few of them are like test_number_when_precision_none_n_invalid_scale while others are like test_number_when_scale_none_n_valid_precision. Please follow a consistent convention over here, preferably add precision first and then scale.

@coveralls
Copy link

coveralls commented Sep 28, 2016

Coverage Status

Changes Unknown when pulling af74154 on nareshnootoo:feature into * on alecthomas:master*.

Copy link
Collaborator

@tusharmakkar08 tusharmakkar08 left a comment

Choose a reason for hiding this comment

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

Please squash the commits. For squashing please refer: http://stackoverflow.com/a/5189600/1799475

assert False, "Did not raise Invalid for String"


def test_number_validation_with_valid_precision_scale_yield_decimal_none():
Copy link
Collaborator

Choose a reason for hiding this comment

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

yield_decimal_false

Copy link
Contributor Author

@nareshnootoo nareshnootoo Sep 28, 2016

Choose a reason for hiding this comment

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

@tusharmakkar08 there is already a test case named test_number_validation_with_valid_precision_scale_yield_decimal_false
check the last test case

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean add for yield_decimal_true since this test is same as test at Line 652.

Added test cases for Number validations

1) test cases for while scale is None,  precision is None,  Both are None
2) Beaking down of Invalid conditions

removed extra space

1) Used Decimal and string conversions
2) __get_precision_scale as private

Modification in comment

Removed InvalidOperation from import

Change is class comment

Made some changes in test cases

class and __call__ method description

Fixed class description

Fixed class description

1) Moved Decimal import up
2) Added InvalidOperation exception instead of generic
3) Made __get_precision_scale to internal as _get_precision_scale
4) Added assert for doctests
5) Added case with only invalid precision and no scale

Added assert for doctests

1) Added Yeild flag(decimal/string)
2) Fixed tests naming convention

Fixed exception
@coveralls
Copy link

coveralls commented Sep 28, 2016

Coverage Status

Changes Unknown when pulling 4a64385 on nareshnootoo:feature into * on alecthomas:master*.

@tusharmakkar08
Copy link
Collaborator

Hey @nareshnootoo

The commits haven't been squashed (Also fix the comment in my last review). After doing rebase, you might need to do git push -f since if you will pull, it will pull all the old commits as well.

Thanks

@coveralls
Copy link

coveralls commented Sep 28, 2016

Coverage Status

Changes Unknown when pulling 4a1230d on nareshnootoo:feature into * on alecthomas:master*.

1 similar comment
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 4a1230d on nareshnootoo:feature into * on alecthomas:master*.

@tusharmakkar08
Copy link
Collaborator

Hey @nareshnootoo

Please squash the changes and get this merged into master 🍰

Thanks

Added test cases for Number validations

1) test cases for while scale is None,  precision is None,  Both are None
2) Beaking down of Invalid conditions

1) Used Decimal and string conversions
2) __get_precision_scale as private

Modification in comment

Made some changes in test cases

Fixed class description

1) Moved Decimal import up
2) Added InvalidOperation exception instead of generic
3) Made __get_precision_scale to internal as _get_precision_scale
4) Added assert for doctests
5) Added case with only invalid precision and no scale

Added assert for doctests

1) Added Yeild flag(decimal/string)
2) Fixed tests naming convention

Fixed exception

Fixed test case naming
@coveralls
Copy link

coveralls commented Sep 29, 2016

Coverage Status

Coverage increased (+0.04%) to 94.731% when pulling a51b20c on nareshnootoo:feature into 987fd98 on alecthomas:master.

@nareshnootoo
Copy link
Contributor Author

Hi @tusharmakkar08 I have squashed the commits.

@tusharmakkar08 tusharmakkar08 merged commit 8f5a41b into alecthomas:master Sep 29, 2016
@tusharmakkar08
Copy link
Collaborator

Merged 🍰

@nareshnootoo
Copy link
Contributor Author

Thanks :)

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.

3 participants