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

Support annotated_types in st.from_type #3780

Merged
merged 20 commits into from
Nov 16, 2023

Conversation

Viicos
Copy link
Contributor

@Viicos Viicos commented Nov 1, 2023

Fixes #3356.

This is very much work in progress for now

@Viicos Viicos marked this pull request as draft November 1, 2023 20:04
@Zac-HD
Copy link
Member

Zac-HD commented Nov 3, 2023

I think we should carefully avoid changing how strategies are inferred, and only use filters for this. Otherwise, we'll break the ability to register custom strategies for any of the types we expect might be annotated, such as datetimes! Implementation-wise, we'd limit our changes to:

annotated_type = args[0]
return st.from_type(annotated_type)

such that the only new code we'd have looks something like:

annotated_type, *constraints = args 
strategy = st.from_type(annotated_type)
for pred in constraints_to_filter_predicates:
    strategy = strategy.filter(pred)  # rewriting usually makes this efficient
return strategy

The contribution of this PR would thus be to implement constraints_to_filter_predicates() (the CONSTRAINTS_FILTER_MAP is great!), and to write tests which check that we only generate valid inputs from constrained types.

Once merged, I'll write up an issue for the feature-rewriting we'll want to add to make e.g. length bounds more efficient - the trick is that we can convert the constraints to callable objects which can also be recognised by the various filter methods. I think we can also rewrite length-constraining lambdas, so long as we keep the predicate just in case len was redefined... but that's for later.

@Viicos
Copy link
Contributor Author

Viicos commented Nov 3, 2023

This sounds reasonable, the only issue is having timezones as a filter to times()/datetimes() can't be done without giving a timezones strategy beforehand (otherwise we would only get naive times/datetimes by default and the filter predicate would never succeed) :/

As you can see what I do currently is only special case times and datetimes to get the base strategy, otherwise I switch to from_type.

@Zac-HD
Copy link
Member

Zac-HD commented Nov 4, 2023

Ouch, yeah. Let's dodge that by just not supporting Timezone metadata in this PR, and maybe emitting a warning 😅

I'm keen to work it out eventually, but imo it's much better to ship the parts we won't regret asap and spend as long as we need on the others.

@Viicos
Copy link
Contributor Author

Viicos commented Nov 5, 2023

Let me know if the actual implementation suits you, I'll then add tests

Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

Looks reasonable! I'll review again once we've got tests and a changelog entry so that CI passes 😁

@Viicos Viicos marked this pull request as ready for review November 6, 2023 19:47
@Viicos
Copy link
Contributor Author

Viicos commented Nov 6, 2023

(Still need to figure out how to test all constraints)

We probably need to tweak CI to test with annotated_types installed. Should I create an extra tox env?

@Zac-HD Zac-HD force-pushed the annotated-types branch 3 times, most recently from 4aae5a0 to 75ec43b Compare November 13, 2023 22:39
@Zac-HD Zac-HD self-assigned this Nov 14, 2023
@Zac-HD Zac-HD force-pushed the annotated-types branch 4 times, most recently from f7a85c8 to 99000b8 Compare November 16, 2023 04:28
@Zac-HD Zac-HD merged commit eebc1c8 into HypothesisWorks:master Nov 16, 2023
47 checks passed
@Viicos Viicos deleted the annotated-types branch November 16, 2023 06:47
@Viicos
Copy link
Contributor Author

Viicos commented Nov 16, 2023

Thanks for finishing the implementation @Zac-HD! I'll try to think about solutions to:

  • make filter rewriting more efficient (e.g. for len constraints)
  • see if we can support timezones

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.

Check for annotated-types constraints in st.from_type(Annotated[T, ...])
2 participants