Skip to content

Remove bare exceptions from 'pandas/tests' to decrease flake8 E722 warnings #22882

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Remove bare exceptions from 'pandas/tests' to decrease flake8 E722 warnings #22882

wants to merge 1 commit into from

Conversation

HaraldNordgren
Copy link

@HaraldNordgren HaraldNordgren commented Sep 28, 2018 •
edited
Loading

Closes #22872

In my experience, the main reson to not catch bare exception is that thet progam has no way of using distinguisging keyboard exception (crtl + C) from regular exception. So I opted for

except Exception:

in cases where it's not clear what exception may happen or function wrapped in a try-except is so large that basically any exception can in different situations happen.

  • closes #xxxx
  • tests added / passed
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

@pep8speaks
Copy link

Hello @HaraldNordgren! Thanks for submitting the PR.

@HaraldNordgren HaraldNordgren changed the title Remove bare exception from 'pandas/tests' to decrease flake8 E722 warnings Remove bare exceptions from 'pandas/tests' to decrease flake8 E722 warnings Sep 28, 2018
@HaraldNordgren HaraldNordgren force-pushed the pandas/test_bare_exception branch from 4f7a7e6 to 37c6d0b Compare September 28, 2018 20:56
WillAyd
WillAyd requested changes Sep 28, 2018
View reviewed changes
Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

The point of this is to be explicit about which exceptions need to be caught, so except Exception isn't what we'd be looking for. Can you review those in more detail and see if we can choose a more specific type of exception?

@WillAyd WillAyd added Error Reporting Incorrect or improved errors from pandas Clean labels Sep 28, 2018
jbrockmendel
jbrockmendel reviewed Sep 28, 2018
View reviewed changes
pandas/tests/test_nanops.py Outdated
@@ -141,12 +141,12 @@ def _coerce_tds(targ, res):
if axis != 0 and hasattr(
targ, 'shape') and targ.ndim and targ.shape != res.shape:
res = np.split(res, [targ.shape[0]], axis=0)[0]
except:
except (np.ValueError, IndexError):
Copy link
Member

Choose a reason for hiding this comment

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

is np.ValueError different from ValueError?

Copy link
Author

Choose a reason for hiding this comment

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

No, my mistake. numpy.ValueError does not exists. Changed it to ValueError.

@HaraldNordgren HaraldNordgren force-pushed the pandas/test_bare_exception branch 2 times, most recently from 91d922a to a2d30d5 Compare September 29, 2018 15:13
@codecov
Copy link

codecov bot commented Sep 29, 2018 •
edited
Loading

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #22882   +/-   ##
=======================================
  Coverage   92.18%   92.18%           
=======================================
  Files         169      169           
  Lines       50830    50830           
=======================================
  Hits        46860    46860           
  Misses       3970     3970
Flag Coverage Δ
#multiple 90.6% <ø> (ø) ⬆️
#single 42.37% <ø> (ø) ⬆️

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 f4fae35...759e928. Read the comment docs.

@HaraldNordgren HaraldNordgren force-pushed the pandas/test_bare_exception branch 3 times, most recently from bb6f71c to b3a4dcc Compare September 30, 2018 13:56
@HaraldNordgren HaraldNordgren force-pushed the pandas/test_bare_exception branch from b3a4dcc to 759e928 Compare September 30, 2018 14:36
@HaraldNordgren
Copy link
Author

HaraldNordgren commented Sep 30, 2018 •
edited
Loading

@WillAyd Added some more specific errors. Do you think it looks better or would you like to go deeper?

In cases like https://github.com/pandas-dev/pandas/pull/22882/files#diff-45436d9cd11edcc7297e0dc3d699a328R217 I'm not sure which exceptions to catch? E.g. When changing it to (KeyError, AttributeError) the tests fail.

@WillAyd
Copy link
Member

WillAyd commented Sep 30, 2018

@HaraldNordgren I still think it's worthwhile to go deeper.

What failures are you getting in your aforementioned example? Are the errors not clueing you in further on what exceptions to catch?

jreback
jreback requested changes Oct 7, 2018
View reviewed changes
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.

pls be specific on the exceptions rather than using Exception. IOW only if we have a long list of exceptions to catch will we use Exception (so it should be rare)

pandas/tests/indexing/common.py
@@ -214,7 +214,7 @@ def _print(result, error=None):

try:
xp = self.get_result(obj, method2, k2, a)
except:
except Exception:
Copy link
Contributor

Choose a reason for hiding this comment

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

can you be more specific here

pandas/tests/io/formats/test_format.py
@@ -452,7 +452,7 @@ def test_to_string_repr_unicode(self):
for line in rs[1:]:
try:
line = line.decode(get_option("display.encoding"))
except:
except Exception:
Copy link
Contributor

Choose a reason for hiding this comment

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

same

pandas/tests/io/test_pytables.py
pass


def safe_close(store):
try:
if store is not None:
store.close()
except:
except Exception:
Copy link
Contributor

Choose a reason for hiding this comment

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

same

@datapythonista
Copy link
Member

@HaraldNordgren can you make the requested fixes?

@jreback
Copy link
Contributor

jreback commented Nov 1, 2018

can you merge master and update

@datapythonista
Copy link
Member

I think this is fixing the same problems than #23370, which is ready to be merged. Closing as duplicate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbrockmendel jbrockmendel jbrockmendel left review comments

@WillAyd WillAyd WillAyd requested changes

@jreback jreback jreback requested changes

Assignees
No one assigned
Labels
Clean Error Reporting Incorrect or improved errors from pandas
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replace bare excepts by explicit excepts in pandas/tests/
6 participants
@HaraldNordgren @pep8speaks @WillAyd @datapythonista @jreback @jbrockmendel