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

Type-annotate pytest.{exit,skip,fail,xfail,importorskip,warns,raises} #5593

Merged
merged 10 commits into from
Jul 16, 2019

Conversation

bluetech
Copy link
Member

To kickstart the plan laid out in #3342 (comment), this adds type annotations to some of the more well-known pytest APIs.

The PR is split into commits for easier reviewing.

For pytest.raises, there are two approaches, one is more precise but a bit more complex, requiring generics. I first implement the simple approach, then switch to the more complex approach in the last commit, so that it can be more easily dropped if wanted.

I guess at this point the pytest devs will want to chime in on whether they want to go down the static typing path :)

src/_pytest/outcomes.py Outdated Show resolved Hide resolved
src/_pytest/_code/code.py Show resolved Hide resolved
src/_pytest/python_api.py Outdated Show resolved Hide resolved
src/_pytest/python_api.py Show resolved Hide resolved
src/_pytest/_code/code.py Outdated Show resolved Hide resolved
src/_pytest/python_api.py Outdated Show resolved Hide resolved
src/_pytest/python_api.py Outdated Show resolved Hide resolved
src/_pytest/recwarn.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 11, 2019

Codecov Report

Merging #5593 into features will decrease coverage by 0.01%.
The diff coverage is 97.5%.

Impacted file tree graph

@@             Coverage Diff              @@
##           features    #5593      +/-   ##
============================================
- Coverage     96.05%   96.04%   -0.02%     
============================================
  Files           117      117              
  Lines         25652    25748      +96     
  Branches       2502     2505       +3     
============================================
+ Hits          24641    24729      +88     
- Misses          704      708       +4     
- Partials        307      311       +4
Impacted Files Coverage Δ
src/_pytest/outcomes.py 95.52% <100%> (+0.13%) ⬆️
src/_pytest/_code/code.py 94.57% <100%> (+0.27%) ⬆️
src/_pytest/python_api.py 97.35% <100%> (+0.31%) ⬆️
testing/code/test_excinfo.py 96.58% <100%> (+0.02%) ⬆️
testing/test_recwarn.py 99.14% <80%> (-0.42%) ⬇️
testing/python/raises.py 93.75% <80%> (-0.56%) ⬇️
src/_pytest/recwarn.py 92.79% <95.91%> (-4.89%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0f8b462...11f1f79. Read the comment docs.

src/_pytest/_code/code.py Show resolved Hide resolved
src/_pytest/python_api.py Outdated Show resolved Hide resolved
src/_pytest/python_api.py Outdated Show resolved Hide resolved
src/_pytest/recwarn.py Outdated Show resolved Hide resolved
extend-ignore adds ignores in addition to flake8's existing ignores.

The default ignores currently are:
E121,E123,E126,E226,E24,E704,W503,W504
@bluetech
Copy link
Member Author

Updated per @asottile's comments.

…ord arguments"

This reverts commit dfe54cd.

The idea in the commit was to simplify the code by removing the check
and instead letting it TypeError which has the same effect.

However this type error is caught by mypy, and rather than ignoring the
error we think it's better and clearer to go back to the previous
explicit check.
This way, in

    with pytest.raises(ValueError) as cm:
        ...

cm.value is a ValueError and not a BaseException.
Mypy doesn't like calling __init__() in this way.
This way the ExceptionInfo generic parameter can be inferred from the
passed-in exc_info. See for example the replaced cast().
Copy link
Member

@asottile asottile left a comment

Choose a reason for hiding this comment

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

@bluetech
Copy link
Member Author

I'm going to take the "better ask for forgiveness" approach and merge this given the approval from @asottile - please let me know if that's not appropriate.

@bluetech bluetech merged commit faf222f into pytest-dev:features Jul 16, 2019
@asottile
Copy link
Member

@bluetech should these go into master instead of features -- feels like it would be easier to iterate on them in master and they aren't (shouldn't be) breaking

@bluetech
Copy link
Member Author

@asottile Would you mind explaining the difference a bit more? CONTRIBUTING.md says

Target master for bugfixes and doc changes.

Target features for new features or functionality changes.

This doesn't fit any of the options exactly. From what I gather, master is used for patch releases, and it's better not to involve unrelated changes in that, no?

@asottile
Copy link
Member

hmm yeah that's a good point, our CONTRIBUTING.md should probably be updated

generally we use features for any new behaviour or breaking changes (in the case of major release) and then everything else goes into master (this would be a .trivial.rst (internal-only) changelog type imo so it would fit in with master)

@bluetech
Copy link
Member Author

OK, got it.

I have some more PRs planned - do you think I should prepare a PR for master which cherry-picks all of the typing changes up to now, or continue working on features for now (until the next minor release)?

@bluetech bluetech deleted the type-annotations-1 branch February 28, 2020 11:59
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.

4 participants