-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix(types): type hints from future python versions #5693
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
Open
InvincibleRMC
wants to merge
23
commits into
pybind:master
Choose a base branch
from
InvincibleRMC:fix-remaining-future-type-hints
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 15 commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
f430803
fix future type hints
InvincibleRMC 14f9c7d
Merge branch 'master' into fix-remaining-future-type-hints
InvincibleRMC e374a2f
style: pre-commit fixes
pre-commit-ci[bot] 325f080
remove unused var
InvincibleRMC 6dcc1d9
remove union_helper
InvincibleRMC 8570b25
fix speelling error
InvincibleRMC 5ea93d4
base case for union_concat
InvincibleRMC f514886
style: pre-commit fixes
pre-commit-ci[bot] 0d2de7a
add case for one descr
InvincibleRMC c489b95
weakref and final test
InvincibleRMC 55c2999
Merge branch 'master' into fix-remaining-future-type-hints
InvincibleRMC 41e2bbe
Add acrpss_version_type_hint_checker
InvincibleRMC bd41651
cleanup
InvincibleRMC 3d4185a
style: pre-commit fixes
pre-commit-ci[bot] df38f99
remove test.pyi
InvincibleRMC eecf91d
Merge branch 'master' into fix-remaining-future-type-hints
InvincibleRMC 8746f74
use new unions and add fixture
InvincibleRMC d2c2e60
Merge branch 'master' into fix-remaining-future-type-hints
InvincibleRMC f733d21
timohl suggested cleanup
InvincibleRMC 62a2ee6
style: pre-commit fixes
pre-commit-ci[bot] 06a4188
add missing auto
InvincibleRMC c287018
style: pre-commit fixes
pre-commit-ci[bot] 08ff0d3
move operator| def
InvincibleRMC File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, never import from conftest.py. You can put fixtures, etc. there, but don't import from it. It causes all sorts of issues (such as the issue holding up cibuildwheel 3.0 in numpy right now!); it's not meant to be an importable module for users.
We sadly have one case(tests/test_docs_advanced_cast_custom.py
), but I'd like to get rid of that, not add more. Edit: it's only at type check time!Instead of adding a
test/utils
directory just for this, and requiring PyTest 7+, this can be wrapped in a fixture for now.Since we do have one case of importing from conftest, I'm also okay to leave this in for now and add the test utils in a followup later, as anything broken by it will already have workarounds.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this also reduce the quality of assertion rewriting?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, wrapping it in a fixture could look like:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to clarify and to learn from it (since I added that conftest.py import in
tests/test_docs_advanced_cast_custom.py
):Would you also discourage from importing conftest.py while using
if TYPE_CHECKING
?Generally, I like having type hints for function arguments for better code readability and navigation.
Vscode/Pylance is capable of matching test arguments to fixtures and show gray type hints without manual type annotations, so I could see not having those in tests.