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

Improve handling of GroupedMetadata #3986

Merged
merged 1 commit into from
May 13, 2024

Conversation

Zac-HD
Copy link
Member

@Zac-HD Zac-HD commented May 13, 2024

Fixes #3980, closes #3981.

Motivated by pydantic/pydantic#4682 (comment); supporting nested-GroupedMetadata and short-circuiting if such an item contains an st.SearchStrategy will allow for extensive control purely from downstream code.

Comment on lines +298 to +301
elif getattr(arg, "__is_annotated_types_grouped_metadata__", False):
for subarg in arg:
if getattr(subarg, "__is_annotated_types_grouped_metadata__", False):
yield from _get_constraints(tuple(subarg))
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for my understanding: is this addition (supporting nested grouped metadata) related to pydantic/pydantic#4682 (comment), or is it just something you added as an enhancement?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unrelated bugfix, really.

Copy link
Contributor

@Viicos Viicos left a comment

Choose a reason for hiding this comment

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

For the needs of the Pydantic plugin, this looks good! thx

@Zac-HD Zac-HD merged commit 5983af0 into HypothesisWorks:master May 13, 2024
54 checks passed
@Zac-HD Zac-HD deleted the better-annotated-resolution branch May 13, 2024 16:34
@Viicos
Copy link
Contributor

Viicos commented May 13, 2024

Sorry I misread some bits of the PR, I'm just a bit worried about defining an __iter__ method that yields hypothesis strategies on the Pydantic types because:

  • It breaks type checking, as GroupedMetadata.__iter__ is typed as Iterator[BaseMetadata], so yielding a strategy results in type checking error.
  • It feels a bit weird to have an hypothesis integration implemented on the type class directly, especially if we need to do:
@_dataclasses.dataclass(**_internal_dataclass.slots_true)
class UuidVersion(annotated_types.GroupedMetadata):
    """A field metadata class to indicate a [UUID](https://docs.python.org/3/library/uuid.html) version."""

    uuid_version: Literal[1, 3, 4, 5]

    def __get_pydantic_json_schema__(
        self, core_schema: core_schema.CoreSchema, handler: GetJsonSchemaHandler
    ) -> JsonSchemaValue:
        ...

    def __get_pydantic_core_schema__(self, source: Any, handler: GetCoreSchemaHandler) -> core_schema.CoreSchema:
        ...

    def __iter__(self):
        if "hypothesis" in sys.modules:
            yield some_strategy
        else:
            # What to do? Raise an error?

However, it seems pretty common to have this pattern, e.g. defining __rich_repr__.

I'll start the implementation on the Pydantic side, and see if they are ok with it!

@Zac-HD
Copy link
Member Author

Zac-HD commented May 13, 2024

It breaks type checking, as GroupedMetadata.__iter__ is typed as Iterator[BaseMetadata], so yielding a strategy results in type checking error.

The readme types it as Iterator[object], and says "libraries consuming annotated-types constraints should check for GroupedMetadata and unpack it by iterating over the object and treating the results as if they had been "unpacked" in the Annotated type".

So actually I have two followups:

  1. fix the annotation in the definition of GroupedMetadata to match the docs (in annotated-types)
  2. add another clause to handle Unpack the same way as GroupedMetadata (in Hypothesis)

@Viicos
Copy link
Contributor

Viicos commented May 13, 2024

Ah yes sorry was looking at the actual type definition indeed!

@dvir478
Copy link

dvir478 commented May 18, 2024

I'm partner dvir478 is working on issue #3986

@Esra-Al
Copy link

Esra-Al commented May 18, 2024

I am Esra-Al and working on issue #3986

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.

Small optimization in find_annotated_strategy
4 participants