Skip to content

Conversation

hmvp
Copy link
Contributor

@hmvp hmvp 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 add_conlist branch 2 times, most recently from 5a43ffc to 1f1be1e Compare June 7, 2019 23:32
@codecov
Copy link

codecov bot 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
Copy link
Contributor Author

hmvp 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

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@hmvp hmvp force-pushed the add_conlist branch 2 times, most recently from 81296a9 to dc64b32 Compare June 11, 2019 09:04
@hmvp
Copy link
Contributor Author

hmvp 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...

@samuelcolvin
Copy link
Member

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
Copy link
Contributor Author

hmvp 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
Copy link
Member

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

@hmvp
Copy link
Contributor Author

hmvp 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
Copy link
Contributor

@dmontagu dmontagu Jun 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe something like

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

class ConstrainedList(ConstrainedListSuperType):
    ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe something like

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

class ConstrainedList(ConstrainedListSuperType):
    ...

hmvp and others added 2 commits June 18, 2019 22:50
Co-Authored-By: Samuel Colvin <samcolvin@gmail.com>
Co-Authored-By: Samuel Colvin <samcolvin@gmail.com>
@samuelcolvin samuelcolvin merged commit 1c45373 into pydantic:master Jun 19, 2019
@samuelcolvin
Copy link
Member

awesome. Thanks so much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants