Skip to content

STY: use pytest.raises context syntax #24297

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

Merged
merged 6 commits into from
Dec 16, 2018
Merged

Conversation

jbrockmendel
Copy link
Member

There are still about a thousand non-context usages of pytest.raises.

@gfyoung if there is a subset of these that especially merit match kwargs, let me know. I'm not eager to manually go through all of them, but doing something systematic or manually doing a few I'd be up for.

@pep8speaks
Copy link

Hello @jbrockmendel! Thanks for submitting the PR.

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

can you create a lint rule to check for non-context manager usage of pytest.raises?

# - assign a complete row (mixed values) -> exp_single_row
df = orig.copy()
df.iloc[2, :] = ["b", 2]
tm.assert_frame_equal(df, exp_single_row)

# - assign a complete row (mixed values) not in categories set
def f():
with pytest.raises(ValueError):
np.log(s)
Copy link
Contributor

Choose a reason for hiding this comment

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

what's this

Copy link
Member Author

Choose a reason for hiding this comment

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

must be a copy/paste mixup, will fix

@jreback jreback added the Code Style Code style, linting, code_checks label Dec 15, 2018
@codecov
Copy link

codecov bot commented Dec 15, 2018

Codecov Report

Merging #24297 into master will decrease coverage by 49.27%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master   #24297       +/-   ##
===========================================
- Coverage   92.28%      43%   -49.28%     
===========================================
  Files         162      162               
  Lines       51830    51830               
===========================================
- Hits        47831    22290    -25541     
- Misses       3999    29540    +25541
Flag Coverage Δ
#multiple ?
#single 43% <ø> (ø) ⬆️
Impacted Files Coverage Δ
pandas/io/formats/latex.py 0% <0%> (-100%) ⬇️
pandas/core/categorical.py 0% <0%> (-100%) ⬇️
pandas/io/sas/sas_constants.py 0% <0%> (-100%) ⬇️
pandas/tseries/plotting.py 0% <0%> (-100%) ⬇️
pandas/tseries/converter.py 0% <0%> (-100%) ⬇️
pandas/io/formats/html.py 0% <0%> (-98.65%) ⬇️
pandas/core/groupby/categorical.py 0% <0%> (-95.46%) ⬇️
pandas/io/sas/sas7bdat.py 0% <0%> (-91.17%) ⬇️
pandas/io/sas/sas_xport.py 0% <0%> (-90.15%) ⬇️
pandas/core/tools/numeric.py 10.44% <0%> (-89.56%) ⬇️
... and 120 more

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 e3b6683...59cb98b. Read the comment docs.

@codecov
Copy link

codecov bot commented Dec 15, 2018

Codecov Report

Merging #24297 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #24297   +/-   ##
=======================================
  Coverage   92.28%   92.28%           
=======================================
  Files         162      162           
  Lines       51827    51827           
=======================================
  Hits        47827    47827           
  Misses       4000     4000
Flag Coverage Δ
#multiple 90.68% <ø> (ø) ⬆️
#single 43.01% <ø> (ø) ⬆️

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 df3b045...b057554. Read the comment docs.

@gfyoung
Copy link
Member

gfyoung commented Dec 16, 2018

@jbrockmendel : Thanks for the ping. Given how massive this is, I think we can just focus on the conversion to context manager first. Which ones should use a match parameter can be determined after (i.e. subsequent PR’s)

@jreback jreback added this to the 0.24.0 milestone Dec 16, 2018
@jreback jreback merged commit 41780e5 into pandas-dev:master Dec 16, 2018
@jreback
Copy link
Contributor

jreback commented Dec 16, 2018

thanks. @gfyoung we should prob have a lint rule to detect this, but leave commented for now

@jbrockmendel jbrockmendel deleted the raises branch December 16, 2018 21:50
@gfyoung
Copy link
Member

gfyoung commented Dec 17, 2018

@jreback: I actually was discussing the checking if error messages, not the usage of the context manager versus not. That being said, I do agree we should lint for what was addressed here.

gfyoung added a commit to forking-repos/pandas that referenced this pull request Dec 18, 2018
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Style Code style, linting, code_checks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants