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

[signature validation] Skip TypeVar signatures #164

Merged

Conversation

david-caro
Copy link
Contributor

In order to be able to check TypeVars we have to be able to check the
whole signature type hints at the same time (as typevars are dynamically
defined).

This just skips them for now to avoid failing the type checks.

It's using the str method of the type hint to traverse the type
definition and see if there are any typevars (prefixed with ~), it's a
hacky way, but quite effective until we have the need to traverse the
type hint ourselves, at that point we can use that instead.

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Apr 18, 2020
testslide/lib.py Outdated Show resolved Hide resolved
testslide/lib.py Outdated Show resolved Hide resolved
.gitignore Show resolved Hide resolved
testslide/lib.py Outdated
@@ -82,17 +82,33 @@ def _validate_function_signature(
expected_type = argspec.annotations.get(argname)
if not expected_type:
continue

if '~' in str(expected_type):
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a more thorough way of detecting this? Eg: isinstance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that it can be nested, like Type[~Something], Optional[Tuple[int, Tuple[str, ~Something]]], and I did not want to have to implement the unwrapping of that, but as str already does and ~ is kinda unique, seems like a good trade-off.

But yes, I think that eventually we will have to unwrap that, at that point we can use that here instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's reasonable for the time being, NP.

Copy link
Contributor

@fornellas fornellas left a comment

Choose a reason for hiding this comment

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

LGTM, just please fix the nits & builds before merging (black should do it).

I registered #165 to eventually implement this. Thanks @david-caro !

In order to be able to check TypeVars we have to be able to check the
whole signature type hints at the same time (as typevars are dynamically
defined).

This just skips them for now to avoid failing the type checks.

It's using the `str` method of the type hint to traverse the type
definition and see if there are any typevars (prefixed with `~`), it's a
hacky way, but quite effective until we have the need to traverse the
type hint ourselves, at that point we can use that instead.
@david-caro david-caro force-pushed the skip_typevar_signature_checks branch from 57646a7 to d8cc163 Compare April 18, 2020 11:31
@david-caro david-caro merged commit 3cbccd0 into facebook:master Apr 18, 2020
@david-caro david-caro deleted the skip_typevar_signature_checks branch April 18, 2020 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants