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

Some type annotation & check_untyped_defs fixes #6311

Merged
merged 5 commits into from Jan 19, 2020

Conversation

bluetech
Copy link
Member

@bluetech bluetech commented Dec 3, 2019

The first two commits mostly finish annotating _pytest/code/.

The other commits fix check_untyped_defs errors in a few test files. This discovered a couple of problems in the annotations, which are fixed.

Most of the diff is adding -> None to test functions. Also, because of a deficiency in mypy, exceptions defined on functions e.g. pytest.fail.Exception don't work, so I replace them with the direct classes instead.

@bluetech bluetech force-pushed the type-annotations-10 branch 2 times, most recently from 8a3c06e to 838f7bf Compare December 5, 2019 12:46
testing/test_pytester.py Outdated Show resolved Hide resolved
@bluetech
Copy link
Member Author

bluetech commented Jan 1, 2020

Rebased on latest features (no conflicts).

@blueyed
Copy link
Contributor

blueyed commented Jan 14, 2020

Most of the diff is adding -> None to test functions.

I'd rather see this enabled via mypy's config (see #6452), mostly to avoid diff churn.
What do you think?

Copy link
Contributor

@blueyed blueyed left a comment

Choose a reason for hiding this comment

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

Let's get this merged finally.
(needs a rebase though)
Would also be interesting to see how this behaves with #6486, but should not hold it back any longer.

src/_pytest/_code/source.py Show resolved Hide resolved
recorder.hook.pytest_runtest_logreport(report=rep)
rep2.passed = False
rep2.skipped = True
recorder.hook.pytest_runtest_logreport(report=rep2)
Copy link
Contributor

Choose a reason for hiding this comment

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

Diff churn like this makes me think that --allow-redefinition would be better (docs).
If not for "src" then maybe for "testing" at least, and maybe only per module there then?

Copy link
Member Author

Choose a reason for hiding this comment

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

I personally think this makes the code less confusing. I generally prefer to fix than to use allow-redefinition, for clarity and for the occasional errors it catches. Though it might be slightly annoying sometimes.

testing/test_runner.py Show resolved Hide resolved
@blueyed
Copy link
Contributor

blueyed commented Jan 17, 2020

@bluetech
I also think it makes sense to rebase this against master.

Also replace one direct call to `compile` with this flag with the
equivalent wrapper `ast.parse`. This function can have a more precise
type.
These are more "dirty" than the previous batch (that's why they were
left out). The trouble is that `compile` can return either a code object
or an AST depending on a flag, so we need to add an overload to make the
common case Union free. But it's still worthwhile.
@bluetech bluetech changed the base branch from features to master January 19, 2020 18:00
@bluetech bluetech merged commit 44eb1f5 into pytest-dev:master Jan 19, 2020
@bluetech bluetech deleted the type-annotations-10 branch January 19, 2020 18:17
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.

None yet

2 participants