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 generic types in signature inspection #2640

Conversation

rcarriga
Copy link
Contributor

The code #2580 was taken from existing code that was intended for an older Python version which didn't take the typing module into account. This patch removes the check for type instance, to allow the typing types such as Literal, Union, etc.

As the signature is only manually created, I don't see an issue with the complete removal of the check. The alternative is to ensure it is from the typing module but I can't find an easy way to do this without using the private types.

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.

Thanks for pointing this out, and for the patch!

I'm going to ask for something a little more complicated, though: because type annotations aren't the only possible use of function argument annotations, we'll still need to check that the thing "is a type (semantically)". Fortunately we already have the helper function for that:

def is_a_type(thing):
"""Return True if thing is a type or a generic type like thing."""
return isinstance(thing, type) or is_generic_type(thing) or is_a_new_type(thing)

You'll probably need to import it locally inside the function though, to avoid import cycles. And then finally, we'll want a test that given e.g. def f(a: 1): ..., we don't treat that annotation as a type.

hypothesis-python/tests/nocover/test_build_signature.py Outdated Show resolved Hide resolved
hypothesis-python/RELEASE.rst Outdated Show resolved Hide resolved
hypothesis-python/src/hypothesis/internal/compat.py Outdated Show resolved Hide resolved
@Zac-HD Zac-HD added the enhancement it's not broken, but we want it to be better label Oct 13, 2020
@rcarriga rcarriga force-pushed the fix/remove-signature-type-instance-check branch from 17cbd90 to d2ca4b8 Compare October 13, 2020 11:17
@rcarriga
Copy link
Contributor Author

Did not realise that is_a_type existed, that's perfect!

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.

Nice! Two minor issues below; but you're pretty much done 😁

Most of the test failures are because Lark just released a new version which turns out to break our integration; I'll be fixing that in #2639 tomorrow.

@Zac-HD Zac-HD force-pushed the fix/remove-signature-type-instance-check branch from 718b85f to 2b4291e Compare October 15, 2020 02:22
@Zac-HD Zac-HD merged commit 05df8ba into HypothesisWorks:master Oct 15, 2020
@rcarriga rcarriga deleted the fix/remove-signature-type-instance-check branch October 15, 2020 15:31
@Zac-HD Zac-HD changed the title remove instanceof type check for signature hints Support generic types in signature inspection Oct 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement it's not broken, but we want it to be better
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants