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

Fix #609 (default arguments for GenericModel) #610

Merged
merged 8 commits into from Jun 25, 2019

Conversation

Projects
None yet
2 participants
@dmontagu
Copy link
Contributor

commented Jun 21, 2019

Fix #609 (default arguments for GenericModel)

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • HISTORY.rst has been updated
Show resolved Hide resolved tests/test_generics.py
Show resolved Hide resolved pydantic/generics.py
@codecov

This comment has been minimized.

Copy link

commented Jun 21, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (master@3ee54ed). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             master   #610   +/-   ##
=======================================
  Coverage          ?   100%           
=======================================
  Files             ?     15           
  Lines             ?   2582           
  Branches          ?    510           
=======================================
  Hits              ?   2582           
  Misses            ?      0           
  Partials          ?      0
@samuelcolvin
Copy link
Owner

left a comment

maybe we should add a test for the case of a default as per the issue

def test_default_arguments():
    T = TypeVar('T')

    class Result(GenericModel, Generic[T]):
        data: T
        other: bool = True
    result = Result[int](data=1)

?

Show resolved Hide resolved tests/test_generics.py Outdated
Show resolved Hide resolved tests/test_generics.py Outdated
@dmontagu

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

maybe we should add a test for the case of a default as per the issue

def test_default_arguments():
    T = TypeVar('T')

    class Result(GenericModel, Generic[T]):
        data: T
        other: bool = True
    result = Result[int](data=1)

?

Sorry my comments were confusing -- when I said I added tests not directly related, I meant in addition to one that was.

The following test was already committed:

@skip_36
def test_default_arguments():
    T = TypeVar('T')

    class Result(GenericModel, Generic[T]):
        data: T
        other: bool = True

    result = Result[int](data=1)
    assert result.other is True

Let me know if that's not enough

@samuelcolvin

This comment has been minimized.

Copy link
Owner

commented Jun 21, 2019

sound more double quotes I'm afraid :-). Promise I'll do linting for this soon.

Also, what about

class Result(GenericModel, Generic[T]):
    data: T = 4

Does that work fine? Maybe worth adding a test?

@dmontagu dmontagu force-pushed the dmontagu:generic-default branch from a7937a8 to b18da50 Jun 21, 2019

@dmontagu

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

sound more double quotes I'm afraid :-). Promise I'll do linting for this soon.

+1 here's an option

Also, what about

class Result(GenericModel, Generic[T]):
    data: T = 4

Does that work fine? Maybe worth adding a test?

I'm not sure if I should be reading anything special into this (e.g., raise errors if T isn't numeric at model creation time?), but it appears to work correctly (or at least, the same as it would if inheriting from BaseModel with T replaced). I added the test.

David Montague and others added some commits Jun 21, 2019

Update tests/test_generics.py
Co-Authored-By: Samuel Colvin <samcolvin@gmail.com>
Update tests/test_generics.py
Co-Authored-By: Samuel Colvin <samcolvin@gmail.com>
David Montague

@dmontagu dmontagu force-pushed the dmontagu:generic-default branch from b18da50 to d4c4fcf Jun 23, 2019

@samuelcolvin samuelcolvin merged commit 3f754c8 into samuelcolvin:master Jun 25, 2019

5 of 8 checks passed

Header rules No header rules processed
Details
Pages changed 2 new files uploaded
Details
Redirect rules No redirect rules processed
Details
LGTM analysis: Python No new or fixed alerts
Details
Mixed content No mixed content detected
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
samuelcolvin.pydantic Build #20190625.2 succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.