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

Add conlist type #583

Merged
merged 4 commits into from Jun 19, 2019

Conversation

Projects
None yet
3 participants
@hmvp
Copy link
Contributor

commented Jun 7, 2019

Change Summary

Added a conlist type, which allows you to restrict the number of items in a list

Related issue number

#161 Is related to this

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
    • if this is the first change since a release, please add a new section
    • include the issue number or this pull request number #<number>
    • include your github username @<whomever>

@hmvp hmvp force-pushed the hmvp:add_conlist branch 2 times, most recently from 5a43ffc to 1f1be1e Jun 7, 2019

@codecov

This comment has been minimized.

Copy link

commented Jun 7, 2019

Codecov Report

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

@@          Coverage Diff          @@
##           master   #583   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          15     15           
  Lines        2542   2582   +40     
  Branches      504    510    +6     
=====================================
+ Hits         2542   2582   +40

@hmvp hmvp force-pushed the hmvp:add_conlist branch from 1f1be1e to 1c8fc74 Jun 8, 2019

@hmvp

This comment has been minimized.

Copy link
Contributor Author

commented Jun 8, 2019

I have no clue why all the tests are failing and why the coverage is reduced. I cannot really reproduce it locally except for the Cython issue... It seems Cython doesn't like the Type[T] type, without type arguments however I get another error

@samuelcolvin
Copy link
Owner

left a comment

looking good, thank you. Nothing major to fix, just some small things.

Show resolved Hide resolved pydantic/fields.py Outdated
Show resolved Hide resolved pydantic/fields.py Outdated
Show resolved Hide resolved pydantic/types.py Outdated
Show resolved Hide resolved pydantic/types.py Outdated
Show resolved Hide resolved pydantic/types.py Outdated
Show resolved Hide resolved pydantic/types.py Outdated
Show resolved Hide resolved tests/test_types.py Outdated
Show resolved Hide resolved tests/test_types.py

@hmvp hmvp force-pushed the hmvp:add_conlist branch 2 times, most recently from 81296a9 to dc64b32 Jun 11, 2019

@hmvp

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

Hmm one test is still failing on 3.6.. I don't have access to 3.6 atm. So it is a bit hard to debug that one. I am going to leave this open until I have time for this or until someone else finishes this...

@hmvp hmvp force-pushed the hmvp:add_conlist branch from dc64b32 to 58485ac Jun 11, 2019

@samuelcolvin

This comment has been minimized.

Copy link
Owner

commented Jun 13, 2019

If you fix the current test that's failing on all versions, I'll fix the 3.6 test as I have it running locally.

@hmvp hmvp force-pushed the hmvp:add_conlist branch from 58485ac to 659e13a Jun 13, 2019

@hmvp

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2019

If you fix the current test that's failing on all versions, I'll fix the 3.6 test as I have it running locally.

Thanks. I fixed the test..

@samuelcolvin

This comment has been minimized.

Copy link
Owner

commented Jun 13, 2019

this is still failing on lots of stuff, I guess related to how cython manages imports.

hmvp added some commits Jun 7, 2019

@hmvp hmvp force-pushed the hmvp:add_conlist branch from 6ce5752 to 214ff90 Jun 17, 2019

@hmvp

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2019

Hmm fixing the Cython issue seems to have fixed 3.6...

class ConstrainedList(List[T]):
__origin__ = list # Needed for pydantic to detect that this is a list
# This types superclass should be List[T], but cython chokes on that...
class ConstrainedList(list): # type: ignore

This comment has been minimized.

Copy link
@dmontagu

dmontagu Jun 17, 2019

Contributor

I wonder if there is a way to put something into a conditional block (possibly even with if TYPE_CHECKING) to get this to behave with Cython, but so that we don't have to give up the generic parameter for mypy type-checking purposes. Or maybe it can still just be type-hinted as List[T]?

This comment has been minimized.

Copy link
@samuelcolvin

samuelcolvin Jun 18, 2019

Owner

Maybe something like

if TYPE_CHECKING:
    class ConstrainedListSuperType(List[T]):
        pass
else:
    class ConstrainedListSuperType(list):
        pass

class ConstrainedList(ConstrainedListSuperType):
    ...

This comment has been minimized.

Copy link
@hmvp

hmvp Jun 19, 2019

Author Contributor

I tried to implement this but I think I can only put the whole class under a conditional. The above does not work because for the typing to be correct you need: class ConstrainedList(ConstrainedListSuperType[T]): which gives the same issue...

This might be related to cython/cython#2753

This comment has been minimized.

Copy link
@hmvp

hmvp Jun 19, 2019

Author Contributor

I can put the whole block in if TYPE_CHECKING: but that duplicates quite a lot of code. So I would rather do it this way for now...

Show resolved Hide resolved pydantic/fields.py Outdated
class ConstrainedList(List[T]):
__origin__ = list # Needed for pydantic to detect that this is a list
# This types superclass should be List[T], but cython chokes on that...
class ConstrainedList(list): # type: ignore

This comment has been minimized.

Copy link
@samuelcolvin

samuelcolvin Jun 18, 2019

Owner

Maybe something like

if TYPE_CHECKING:
    class ConstrainedListSuperType(List[T]):
        pass
else:
    class ConstrainedListSuperType(list):
        pass

class ConstrainedList(ConstrainedListSuperType):
    ...
Show resolved Hide resolved pydantic/types.py Outdated

hmvp and others added some commits Jun 18, 2019

Update pydantic/fields.py
Co-Authored-By: Samuel Colvin <samcolvin@gmail.com>
Update pydantic/types.py
Co-Authored-By: Samuel Colvin <samcolvin@gmail.com>

@samuelcolvin samuelcolvin merged commit 1c45373 into samuelcolvin:master Jun 19, 2019

5 of 8 checks passed

Header rules No header rules processed
Details
Pages changed All files already uploaded
Details
Redirect rules No redirect rules processed
Details
Mixed content No mixed content detected
Details
codecov/project 100% (+0%) compared to 84820bd
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
samuelcolvin.pydantic Build #20190618.10 succeeded
Details
@samuelcolvin

This comment has been minimized.

Copy link
Owner

commented Jun 19, 2019

awesome. Thanks so much.

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.