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

add ``UUID1``, ``UUID3``, ``UUID4`` and ``UUID5`` types #167

Merged
merged 1 commit into from May 2, 2018

Conversation

2 participants
@Gr1N
Collaborator

Gr1N commented Apr 30, 2018

No description provided.

@Gr1N Gr1N force-pushed the Gr1N:uuid-type branch from a1b2f09 to 8ddd639 Apr 30, 2018

Gr1N added a commit to Gr1N/pydantic that referenced this pull request Apr 30, 2018

@codecov

This comment has been minimized.

codecov bot commented Apr 30, 2018

Codecov Report

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

@@          Coverage Diff          @@
##           master   #167   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           9      9           
  Lines         938    950   +12     
  Branches      206    207    +1     
=====================================
+ Hits          938    950   +12
def conuuid(*, version=4) -> Type[UUID]:
# use kwargs then define conf in a dict to aid with IDE type hinting
namespace = dict(version_=version)
return type('ConstrainedUUIDValue', (ConstrainedUUID,), namespace)

This comment has been minimized.

@samuelcolvin

samuelcolvin May 1, 2018

Owner

Given that there are only 4 times, should it would be cleaner to just have 4 tyes: Uuid1, Uuid2, ...

Rather than having to implement conuuid

This comment has been minimized.

@Gr1N

Gr1N May 1, 2018

Collaborator

You're right!

@Gr1N Gr1N force-pushed the Gr1N:uuid-type branch from 8ddd639 to 08cbca6 May 1, 2018

Gr1N added a commit to Gr1N/pydantic that referenced this pull request May 1, 2018

add ``Config.uuid_version`` (samuelcolvin#167)
add ``ConstrainedUUID``, ``UUID1``, ``UUID3``, ``UUID4`` and ``UUID5`` types (samuelcolvin#167)

@Gr1N Gr1N changed the title from add ``Config.uuid_version``; add ``ConstrainedUUID`` and ``conuuid`` types to add ``Config.uuid_version``; add ``ConstrainedUUID``, ``UUID1``, ``UUID3``, ``UUID4`` and ``UUID5`` types May 1, 2018

@Gr1N

This comment has been minimized.

Collaborator

Gr1N commented May 1, 2018

@samuelcolvin, updated, please review

@samuelcolvin

Looking good, but I think we can make it simpler.

@@ -20,6 +20,7 @@ class BaseConfig:
max_anystr_length = 2 ** 16
min_number_size = -2 ** 64
max_number_size = 2 ** 64
uuid_version = None

This comment has been minimized.

@samuelcolvin

samuelcolvin May 1, 2018

Owner

I think adding this is unnecessarily confusing. The 5 types UUID, UUID1, UUID3, UUID4 and UUID5 are sufficient.

This comment has been minimized.

@samuelcolvin

samuelcolvin May 1, 2018

Owner

Then you can remove the duplicated logic in ConstrainedUUID and uuid_version_validator

This comment has been minimized.

@Gr1N

Gr1N May 1, 2018

Collaborator

Done

@@ -48,7 +52,11 @@ class Model(BaseModel):
neg_float=-2.3,
email_address='Samuel Colvin <s@muelcolvin.com >',
email_and_name='Samuel Colvin <s@muelcolvin.com >',
uuid='ebcdab58-6eb8-46fb-a190-d07a33e9eac8'
uuid='ebcdab58-6eb8-46fb-a190-d07a33e9eac8',
uuid_v1='c96e505c-4c62-11e8-a27c-dca90496b483',

This comment has been minimized.

@samuelcolvin

samuelcolvin May 1, 2018

Owner

It'll be much clearer to read if you show how these were generated, eg.

uuid=uuid.uuid4(),
uuid_v1=uuid.uuid1(),
....
uuid_v=uuid.uuid5(uuid.NAMESPACE_DNS, 'example.com')

This comment has been minimized.

@samuelcolvin

samuelcolvin May 1, 2018

Owner

same in tests.

This comment has been minimized.

@Gr1N

Gr1N May 1, 2018

Collaborator

Done

@@ -237,4 +243,36 @@ class NegativeFloat(ConstrainedFloat):
lt = 0
class ConstrainedUUID(UUID):

This comment has been minimized.

@samuelcolvin

samuelcolvin May 1, 2018

Owner

ConstrainedUUID could lead to confusion, I think we can remove it altogether.

I think best to just have

class UUID1(UUID):
    _required_version = 1

...

Then change uuid_validator:

def uuid_validator(v) -> UUID:
    if isinstance(v, str):
        v = UUID(v)
    elif isinstance(v, (bytes, bytearray)):
        v = UUID(v.decode())
    elif not isinstance(v, UUID):
        raise ValueError(f'str, byte or native UUID type expected not {type(v)}')
    required_version = getattr(v, '_required_version', None)
    if required_version and required_version != v.version:
        ...
    return v

This comment has been minimized.

@Gr1N

Gr1N May 1, 2018

Collaborator

This is a good idea, but I think this is impossible to implement (or I don't know how :). As I see in my tests in uuid_validator v is instance of UUID type, not UUID1 or UUID2...

Validator implementation:

def uuid_validator(v) -> UUID:
    if isinstance(v, str):
        v = UUID(v)
    elif isinstance(v, (bytes, bytearray)):
        v = UUID(v.decode())
    elif not isinstance(v, UUID):
        raise ValueError(f'str, byte or native UUID type expected not {type(v)}')

    required_version = getattr(v, '_required_version', None)
    if required_version and v.version != required_version:
        raise ValueError(f'uuid version {required_version} expected, not {v.version}')

    return v

And test:

class UUIDModel(BaseModel):
    a: UUID1
    b: UUID3
    c: UUID4
    d: UUID5


def test_uuid_validation():
    a = uuid.uuid1()
    b = uuid.uuid3(uuid.NAMESPACE_DNS, 'python.org')
    c = uuid.uuid4()
    d = uuid.uuid5(uuid.NAMESPACE_DNS, 'python.org')

    m = UUIDModel(a=a, b=b, c=c, d=d)
    assert m.dict() == {
        'a': a,
        'b': b,
        'c': c,
        'd': d,
    }

    with pytest.raises(ValidationError) as exc_info:
        UUIDModel(a=d, b=c, c=b, d=a)
    assert exc_info.value.message == '4 errors validating input'

This comment has been minimized.

@Gr1N

Gr1N May 2, 2018

Collaborator

Done! :)

@Gr1N Gr1N force-pushed the Gr1N:uuid-type branch from 08cbca6 to a049fe5 May 1, 2018

Gr1N added a commit to Gr1N/pydantic that referenced this pull request May 1, 2018

@Gr1N Gr1N changed the title from add ``Config.uuid_version``; add ``ConstrainedUUID``, ``UUID1``, ``UUID3``, ``UUID4`` and ``UUID5`` types to add ``ConstrainedUUID``, ``UUID1``, ``UUID3``, ``UUID4`` and ``UUID5`` types May 1, 2018

@Gr1N Gr1N force-pushed the Gr1N:uuid-type branch from a049fe5 to e23861b May 2, 2018

@Gr1N Gr1N changed the title from add ``ConstrainedUUID``, ``UUID1``, ``UUID3``, ``UUID4`` and ``UUID5`` types to add ``UUID1``, ``UUID3``, ``UUID4`` and ``UUID5`` types May 2, 2018

@Gr1N

This comment has been minimized.

Collaborator

Gr1N commented May 2, 2018

@samuelcolvin please review again!

@samuelcolvin samuelcolvin merged commit b4d3a2d into samuelcolvin:master May 2, 2018

3 checks passed

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

This comment has been minimized.

Owner

samuelcolvin commented May 2, 2018

Looks good, thank you very much. Sorry for the mistake in my comment code!

@Gr1N Gr1N deleted the Gr1N:uuid-type branch May 3, 2018

@Gr1N Gr1N referenced this pull request May 3, 2018

Closed

More exotic types #161

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