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

fix type annotations for exotic types; re-use type validators in exotic types #171

Merged
merged 1 commit into from May 5, 2018

Conversation

2 participants
@Gr1N
Collaborator

Gr1N commented May 3, 2018

Changes:

  • fixed type annotations for exotic types due to new release 0.600 of mypy
  • also I tried to simplify codebase and re-use type validators in exotic types, less code better to my mind, please let me know what do you think and what I need to improve

To discuss:

  • I would like to rename gt and lt in ConstrainedInt and ConstrainedFloat because we already have min_number_size and max_number_size for int and float and to my mind it looks like kind of inconsistency. And I think we can do this backward compatible by adding new min_size and max_size without removing gt and lt. What do you think?

@Gr1N Gr1N force-pushed the Gr1N:simplify-codebase branch from 0d7307d to d762b71 May 3, 2018

@codecov

This comment has been minimized.

codecov bot commented May 3, 2018

Codecov Report

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

@@          Coverage Diff          @@
##           master   #171   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           9      9           
  Lines         950    939   -11     
  Branches      207    200    -7     
=====================================
- Hits          950    939   -11
@Gr1N

This comment has been minimized.

Collaborator

Gr1N commented May 5, 2018

@samuelcolvin ping! :)

@Gr1N Gr1N force-pushed the Gr1N:simplify-codebase branch from d762b71 to 9c17a8f May 5, 2018

@samuelcolvin

Otherwise looking good.

Please can you run benchmarks (python benchmarks/run.py pydantic-only) for master and this branch and include the results here - we should just check that this hasn't caused a significant performance impact.

@@ -211,21 +196,13 @@ class NegativeInt(ConstrainedInt):
class ConstrainedFloat(float):
gt: Union[int, float] = None
lt: Union[int, float] = None
gt: Optional[Union[int, float]] = None

This comment has been minimized.

@samuelcolvin

samuelcolvin May 5, 2018

Owner

Surely cleaner here to use Union[None, int, float]

value = value[:cls.curtail_length]
elif cls.max_length is not None and v_len > cls.max_length:
raise ValueError(f'length greater than maximum allowed: {cls.max_length}')
if cls.curtail_length and v_len > cls.curtail_length:

This comment has been minimized.

@samuelcolvin

samuelcolvin May 5, 2018

Owner

I guess here you can use

if cls.curtail_length and len(value) > cls.curtail_length:
    ...

Since the v_len variable isn't reused.

@Gr1N

This comment has been minimized.

Collaborator

Gr1N commented May 5, 2018

Benchmarks on master branch:

$ python benchmarks/run.py pydantic-only
generating test cases...
                                pydantic time=0.651s, success=48.30%
                                pydantic time=0.688s, success=48.30%
                                pydantic time=0.667s, success=48.30%
                                pydantic time=0.658s, success=48.30%
                                pydantic time=0.664s, success=48.30%
                                pydantic best=0.651s, avg=0.666s, stdev=0.014s

                                pydantic best=21.707μs/iter avg=22.188μs/iter stdev=0.467μs/iter

And for this branch:

$ python benchmarks/run.py pydantic-only
                                pydantic time=0.664s, success=48.30%
                                pydantic time=0.670s, success=48.30%
                                pydantic time=0.677s, success=48.30%
                                pydantic time=0.650s, success=48.30%
                                pydantic time=0.661s, success=48.30%
                                pydantic best=0.650s, avg=0.665s, stdev=0.010s

                                pydantic best=21.678μs/iter avg=22.156μs/iter stdev=0.336μs/iter

@Gr1N Gr1N force-pushed the Gr1N:simplify-codebase branch from 9c17a8f to 3356538 May 5, 2018

@samuelcolvin

This comment has been minimized.

Owner

samuelcolvin commented May 5, 2018

Thanks.

In future please don't amend commits, I squash merge anyway and amending commits makes it harder to see the changes after my comments.

@samuelcolvin samuelcolvin merged commit 0ee1ffa into samuelcolvin:master May 5, 2018

3 checks passed

codecov/project 100% (+0%) compared to 89b37d8
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
@Gr1N

This comment has been minimized.

Collaborator

Gr1N commented May 5, 2018

In future please don't amend commits, I squash merge anyway and amending commits makes it harder to see the changes after my comments.

No problem

@samuelcolvin what do you think about?

I would like to rename gt and lt in ConstrainedInt and ConstrainedFloat because we already have min_number_size and max_number_size for int and float and to my mind it looks like kind of inconsistency. And I think we can do this backward compatible by adding new min_size and max_size without removing gt and lt. What do you think?

@Gr1N Gr1N deleted the Gr1N:simplify-codebase branch May 5, 2018

@samuelcolvin

This comment has been minimized.

Owner

samuelcolvin commented May 5, 2018

I would like to rename gt and lt in ConstrainedInt and ConstrainedFloat because we already have min_number_size and max_number_size for int and float and to my mind it looks like kind of inconsistency. And I think we can do this backward compatible by adding new min_size and max_size without removing gt and lt. What do you think?

Sounds good to me. I would do it so usage of lt an gt raises a helpful exception.

We also need to be careful about the exact meaning min_number_size is kind of the same as gte but not the same as gt.

Eg. with gt=1 would indicate that 1 is not an allowed value, but min_number_size=1 would indicate that 1 is an allowed value.

I think the second case makes more sense but most importantly we should be consistent and explicit.

@Gr1N

This comment has been minimized.

Collaborator

Gr1N commented May 5, 2018

So to be clear. As I understand you're good with proposal and we can replace gt and lt with min_size and max_size, but I want to understand right way to implement those changes.

In code base we already have an issue with gt and lt because we're using >= and <= instead of > and < (https://github.com/samuelcolvin/pydantic/blob/v0.9/pydantic/types.py#L187) and I think we need to fix it.

I see there ways:

  1. Backward incompatible changes: just rename to min_size and max_size.
  2. Backward compatible changes: keep lt and gt, but on code level we will go to convert them to min_size and max_size and write deprecation warning to stdout using warnings package.
  3. Same as 2, but raise an exception in runtime if lt and gt will be used.

From code-level I prefer option 1 (I don't like warnings and less code better), but from community side I think option 2 is better.

What do you think? Maybe I don't understand you and you have better solution?

@samuelcolvin

This comment has been minimized.

Owner

samuelcolvin commented May 5, 2018

Code is currently correct for the names lt and gt, I think. But will need to change to match the new names.

Hence why we need a hard depication lt and gt, because the meaning will change.

Shall I do a pr and you can review?

@Gr1N

This comment has been minimized.

Collaborator

Gr1N commented May 5, 2018

Shall I do a pr and you can review?

Yep!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment