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

Handle type validation for string type hints #333

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

williamlw999-fb
Copy link

@williamlw999-fb williamlw999-fb commented Apr 19, 2022

With from __future__ import annotations, all type hints become strings. The current type validator assumes that what is being checked is a type and not a string, so no types are being validated in these cases.

The fix is to use Python's typing.get_type_hints to evaluate the type hints at runtime

  • Added tests, if you've added code that should be tested
  • Ensured the test suite passes
  • Made sure your code lints
  • Completed the Contributor License Agreement ("CLA")

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Apr 19, 2022
@williamlw999-fb williamlw999-fb linked an issue Apr 19, 2022 that may be closed by this pull request
@coveralls
Copy link

coveralls commented Apr 19, 2022

Pull Request Test Coverage Report for Build 4629934671

  • 6 of 6 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.002%) to 93.73%

Totals Coverage Status
Change from base Build 4554046522: 0.002%
Covered Lines: 2661
Relevant Lines: 2839

💛 - Coveralls

@facebook-github-bot
Copy link

@deathowl has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@deathowl
Copy link
Member

LGTM 👍

@fried
Copy link
Member

fried commented Aug 9, 2022

So this pull just needs to run isort?

Snippet from the failed Checks

Formatting errors found, try running 'make format'.
make: *** [Makefile:114: isort] Error 1
Error: Process completed with exit code 2.

@deathowl
Copy link
Member

just run make format

Summary:
With `from __future__ import annotations`, all type hints become strings. The current type validator assumes that what is being checked is a type and not a string, so no types are being validated in these cases.

The fix is to use Python's typing.get_type_hints to evaluate the type hints at runtime

- [x] Added tests, if you've added code that should be tested
- [x] Ensured the test suite passes
- [x] Made sure your code lints
- [x] Completed the Contributor License Agreement ("CLA")

Pull Request resolved: facebook#333

Test Plan:
Imported from GitHub, without a `Test Plan:` line.

New unit tests were added to check that function parameters and return types were being interpreted correctly when annotated as a string (which is the case with from __future__ import annotations)

Reviewed By: deathowl

Differential Revision: D35778959

Pulled By: williamlw999-fb

fbshipit-source-id: 20ad9fbbb22c8189a9ba650729b99f5c59a24a16
@facebook-github-bot
Copy link

This pull request was exported from Phabricator. Differential Revision: D35778959

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.

Cannot type check with from __future__ import annotations
5 participants