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

Added le and ge bounds to constrained numerics. #194

Merged
merged 4 commits into from Jun 8, 2018

Conversation

@jaheba
Copy link
Contributor

@jaheba jaheba commented Jun 7, 2018

See discussion #174.

@codecov
Copy link

@codecov codecov bot commented Jun 7, 2018

Codecov Report

Merging #194 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #194   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          10     10           
  Lines        1149   1172   +23     
  Branches      214    218    +4     
=====================================
+ Hits         1149   1172   +23

@jaheba
Copy link
Contributor Author

@jaheba jaheba commented Jun 7, 2018

Probably we want to update the docs as well.

Copy link
Owner

@samuelcolvin samuelcolvin left a comment

Otherwise this looks great, just one small change, and also could you update HISTORY.rst.

@Gr1N wrote some of this code so might have some feedback too.

def test_number_gt():
class Model(BaseModel):
a: conint(gt=-1) = 0
class TestNumberBounds:
Copy link
Owner

@samuelcolvin samuelcolvin Jun 7, 2018

I understand where you're coming from, but let's stick with no classes in tests for simplicity.

@samuelcolvin
Copy link
Owner

@samuelcolvin samuelcolvin commented Jun 7, 2018

And docs, yes.

try:
assert new_cls.gt is None or new_cls.ge is None, ('gt', 'ge')
assert new_cls.lt is None or new_cls.le is None, ('lt', 'le')
except AssertionError as exception:
Copy link
Contributor

@Gr1N Gr1N Jun 7, 2018

except AssertionError as e:
   ...
   raise errors.ConfigError(...) from e

@@ -12,6 +12,7 @@
from pydantic import (DSN, UUID1, UUID3, UUID4, UUID5, BaseModel, EmailStr, NameEmail, NegativeFloat, NegativeInt,
PositiveFloat, PositiveInt, PyObject, StrictStr, ValidationError, condecimal, confloat, conint,
constr, create_model)
from pydantic.errors import ConfigError
Copy link
Contributor

@Gr1N Gr1N Jun 7, 2018

from pydantic import ConfigError

@@ -175,19 +175,34 @@ def validate(cls, value, values, **kwarg):
return make_dsn(**kwargs)


class ConstrainedInt(int):
class ConstrainedMeta(type):
Copy link
Contributor

@Gr1N Gr1N Jun 7, 2018

Maybe better to change name of class? Something like ConstrainedNumberMeta or just NumberMeta?

@jaheba
Copy link
Contributor Author

@jaheba jaheba commented Jun 7, 2018

Addressed all comments, I think.

@@ -16,10 +16,12 @@ class Model(BaseModel):
strip_str: constr(strip_whitespace=True)

big_int: conint(gt=1000, lt=1024) = None
# inclusive: conint(ge=1000, le=1024)
Copy link
Owner

@samuelcolvin samuelcolvin Jun 7, 2018

remove

@jaheba
Copy link
Contributor Author

@jaheba jaheba commented Jun 7, 2018

@samuelcolvin samuelcolvin merged commit 3ef5955 into samuelcolvin:master Jun 8, 2018
3 checks passed
@samuelcolvin
Copy link
Owner

@samuelcolvin samuelcolvin commented Jun 8, 2018

great, thank you very much.

@jaheba jaheba deleted the inclusive_ranges branch Jun 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants