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 support for nested generics #1104

Merged
merged 5 commits into from Jan 10, 2020
Merged

Conversation

dmontagu
Copy link
Contributor

@dmontagu dmontagu commented Dec 16, 2019

Change Summary

Modifies GenericModel to behave more similarly to standard python generics, including the ability to reference typevars in nested GenericModels, and to make use of partial specifications with new typevars.

There are a couple outstanding issues:

  • As this PR stands, an error will be raised if you try to instantiate a GenericModel after just passing TypeVars as the parameters. Currently, I believe this is allowed, and will perform the same logic that would happen if you were to use a TypeVar as a field annotation elsewhere (e.g., for purposes of a constraint).

    • I believe the right (backwards-compatible) approach here is to treat unspecified parameters the same way we would treat it as if the TypeVar was used as the annotation. I think this is similar to what we do for Dict, List, etc., since those TypeVars have no constraints and so are basically equivalent to Any.
    • This should be easy to implement, but I wanted to get confirmation before putting in more effort. In particular, I think the "right" implementation would be to allow instantiating before you've provided any concrete types for the parameters, similar to how Dict, etc., work. But right now that raises an error. I personally would consider this a fix rather than a breaking change, but I want to reach consensus before moving forward.
    • (It should also be easy to keep the current behavior, but allow instantiation after explicitly specifying TypeVar parameters, I just think that is a wrong API choice -- the question is whether preserving the current behavior is sufficiently worthwhile.)
  • The generated names come out a little weird when you make use of partially-specified models. It would be nice to fix this, but since 1) this is already an edge case, 2) type naming is usually not critical, and 3) we already allow you to override __concrete_name__, I am happy to leave this to users for now.

Related issue number

Closes #967

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • changes/<pull request or issue id>-<github username>.md file added describing change
    (see changes/README.md for details)

@codecov
Copy link

codecov bot commented Dec 17, 2019

Codecov Report

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

@@          Coverage Diff           @@
##           master   #1104   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          20      20           
  Lines        3450    3463   +13     
  Branches      667     672    +5     
======================================
+ Hits         3450    3463   +13
Impacted Files Coverage Δ
pydantic/generics.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1c49a3f...c4df92d. Read the comment docs.

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.

I'm fine with the generated names being weird, but can we have an example in a test so we're clear.


As this PR stands, an error will be raised if you try to instantiate a GenericModel after just passing TypeVars as the parameters

I'm afraid I don't know exactly what you mean. Could you add an example here so I (and anyone else coming to this PR) is clear what you mean. My suspicion is that, as you say, this can reasonably considered a bug fix, not a change in functionality so is fine here. But's document it in a test and perhaps even in the documentation so everything is clear.

pydantic/generics.py Show resolved Hide resolved


class GenericModel(BaseModel):
__slots__ = ()
__concrete__: ClassVar[bool] = False

def __new__(cls, *args: Any, **kwargs: Any) -> Any:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

(This was only necessary to prevent instantiation with unspecified parameters.)

partial_model = Model[int, BT]
assert partial_model.__name__ == 'Model[int, BT]'
concrete_model = partial_model[str]
assert concrete_model.__name__ == 'Model[int, BT][str]'
Copy link
Contributor Author

@dmontagu dmontagu Jan 10, 2020

Choose a reason for hiding this comment

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

This is what I mean by the names are kind of weird.

It would be a (perhaps surprisingly) involved refactor to make it generate the names properly (by that I mean showing Model[int, str] instead of Model[int, BT][str]). Not impossible though if you'd prefer we make it work.



@skip_36
def test_partial_specification_instantiation_bounded():
Copy link
Contributor Author

@dmontagu dmontagu Jan 10, 2020

Choose a reason for hiding this comment

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

This test and the one below demonstrate that the typevars for (partially-)parametrized models are respected properly. This is a big part of why I think it's okay to drop the check for parametrization in __new__.

@samuelcolvin
Copy link
Member

I think this looks fine, but I'm not going to pretend I've followed every line of the discussion.

Does it need any updates to docs? Otherwise can it be merged?

@dmontagu
Copy link
Contributor Author

dmontagu commented Jan 10, 2020

I think the docs should be updated, but I think it can be a small update. Give me 15 mins and I'll respond here with an estimate of how long it will take to finish.

@dmontagu
Copy link
Contributor Author

@samuelcolvin Okay, I've added docs, let me know if you want any changes.

@samuelcolvin samuelcolvin merged commit dbc044e into pydantic:master Jan 10, 2020
@samuelcolvin
Copy link
Member

awesome, thank you.

andreshndz pushed a commit to cuenca-mx/pydantic that referenced this pull request Jan 17, 2020
* Add support for nested generics

* Allow instantiation of unparameterized generics

* Add better more partial instantiation tests

* Add changes

* Add docs
alexdrydew pushed a commit to alexdrydew/pydantic that referenced this pull request Dec 23, 2023
Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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.

Nested generic models
2 participants