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 InstanceOf type/annotation #5778

Merged
merged 7 commits into from
May 25, 2023
Merged

Add InstanceOf type/annotation #5778

merged 7 commits into from
May 25, 2023

Conversation

dmontagu
Copy link
Contributor

@dmontagu dmontagu commented May 16, 2023

Closes #5773

Looking for feedback on the implementation here before doing documentation/etc.

I've set it up so it has the standard behavior for JSON and JSON schema by default, just uses isinstance for python. We could make this configurable but I wanted to hash out the desired configuration choices first.


Unfortunately, due to the way JSON gets parsed on the Rust side, it seems that we don't get perfect preservation of big-ints, which was one of the things I was hoping to address with this change:

import json

from pydantic import BaseModel
from pydantic.types import SimpleIsInstance


class Model(BaseModel):
    x: SimpleIsInstance[int]


value = dict(x=10000000000000000000000000000000000000000)
print(Model.model_validate(value))
#> x=10000000000000000000000000000000000000000


json_value = json.dumps(value)
print(json_value)
#> {"x": 10000000000000000000000000000000000000000}

print(Model.model_validate_json(json_value))
#> x=9223372036854775807

Not sure if it makes sense to worry about this, but I at least wanted to point it out.

Selected Reviewer: @adriangb

@samuelcolvin
Copy link
Member

Should we just call it IsInstance?

@dmontagu
Copy link
Contributor Author

dmontagu commented May 16, 2023

The only reason I didn't already do that was to avoid conflict with dirty_equals. But it's probably worth it given use of both together should be pretty rare.

I've changed it to that now.

@cloudflare-pages
Copy link

cloudflare-pages bot commented May 16, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 414e034
Status: ✅  Deploy successful!
Preview URL: https://33d1e2ae.pydantic-docs2.pages.dev
Branch Preview URL: https://simple-is-instance.pydantic-docs2.pages.dev

View logs

@dmontagu dmontagu changed the title Add SimpleIsInstance type Add IsInstance type/annotation May 16, 2023
@samuelcolvin
Copy link
Member

IsInstanceOf?

@dmontagu
Copy link
Contributor Author

dmontagu commented May 17, 2023

I think it probably makes sense to just keep it as IsInstance unless you prefer that name @samuelcolvin. (I don't think the conflict with dirty_equals is worth being concerned about, but not sure if IsInstanceOf is preferable for other reasons..)

@dmontagu dmontagu marked this pull request as ready for review May 17, 2023 13:22
@dmontagu
Copy link
Contributor Author

please review

@dmontagu
Copy link
Contributor Author

dmontagu commented May 17, 2023

@adriangb I'm interested in your opinion on this note:

I've set it up so it has the standard behavior for JSON and JSON schema by default, just uses isinstance for python. We could make this configurable but I wanted to hash out the desired configuration choices first.

In particular, we could add arguments to the dataclass and just use "default values" when applied via IsInstance[<type>] (so IsInstance[<type>] is like Annotated[<type>, IsInstance()], but you can manually do Annotated[<type>, IsInstance(disable_json_loading=True)] or similar). But I'm not sure what configuration knobs it makes sense to add, was hoping for insight from you.

(I'm potentially happy to add none and just call this done, but if you have any concerns or good ideas I think we can still augment this a bit.)

@adriangb
Copy link
Member

I'm potentially happy to add none and just call this done, but if you have any concerns or good ideas I think we can still augment this a bit.

It would be backwards compatible to add knobs right? If so I'd say lets not add them for now.

@dmontagu
Copy link
Contributor Author

dmontagu commented May 19, 2023

I could rename to InstanceOf instead of IsInstance? I prefer InstanceOf to IsInstanceOf, but between the two I don't care. I'd like to merge this though when possible.

One thing that might be convenient is to reserve one of these names for use as the annotation type (which may one day accept configuration arguments), and the other for the thing that is treated by typechecker as Annotated[AnyType, ...], as that will ensure that we can add the configuration arguments without breaking type checking in some way.

That way, in the future (not necessary today) it might look like:

@_internal_dataclass.slots_dataclass
class InstanceOf:
    config_flag: bool = False

    @classmethod
    def __class_getitem__(cls, item: AnyType) -> AnyType:
        return Annotated[item, cls()]

    @classmethod
    def __get_pydantic_core_schema__(cls, source: Any, handler: GetCoreSchemaHandler) -> core_schema.CoreSchema:
        # use the generic _origin_ as the second argument to isinstance when appropriate
        python_schema = core_schema.is_instance_schema(get_origin(source) or source)
        json_schema = handler(source)
        return core_schema.json_or_python_schema(python_schema=python_schema, json_schema=json_schema)

if TYPE_CHECKING:
    # If we add configurable attributes to IsInstance, we'd probably need to stop hiding it from type checkers like this
    IsInstance = Annotated[AnyType, ...]  # `IsInstance[Sequence]` will be recognized by type checkers as `Sequence`

else:
    IsInstance = InstanceOf

@dmontagu
Copy link
Contributor Author

please update (need to call it InstanceOf instead of IsInstance and then we'll merge)

@dmontagu
Copy link
Contributor Author

please review

@pydantic-hooky pydantic-hooky bot assigned adriangb and unassigned dmontagu May 23, 2023
@dmontagu dmontagu changed the title Add IsInstance type/annotation Add InstanceOf type/annotation May 23, 2023
@dmontagu dmontagu merged commit e78dd92 into main May 25, 2023
53 checks passed
@dmontagu dmontagu deleted the simple-is-instance branch May 25, 2023 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IsInstance type annotation
3 participants