Skip to content

Fix unbound local with bad engine #16511

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

Merged
merged 3 commits into from
May 31, 2017
Merged

Fix unbound local with bad engine #16511

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9a4f1a7
BUG: Fix uninformative error on bad CSV engine name
jtratner May 26, 2017
35968dc
Move things around as per requested
jtratner May 27, 2017
7c5f2c4
Move one more time
jtratner May 27, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions doc/source/whatsnew/v0.20.2.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ Bug Fixes

- Bug in using ``pathlib.Path`` or ``py.path.local`` objects with io functions (:issue:`16291`)
- Bug in ``DataFrame.update()`` with ``overwrite=False`` and ``NaN values`` (:issue:`15593`)
- Passing an invalid engine to :func:`read_csv` now raises an informative
ValueError rather than UnboundLocalError. (:issue:`16511`)
Copy link
Contributor

Choose a reason for hiding this comment

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

double backtics on ValueError and UnboundLocalError





Expand Down
3 changes: 3 additions & 0 deletions pandas/io/parsers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -969,6 +969,9 @@ def _make_engine(self, engine='c'):
klass = PythonParser
elif engine == 'python-fwf':
klass = FixedWidthFieldParser
else:
raise ValueError('Unknown engine: %r (valid are "c", "python",'
Copy link
Member

Choose a reason for hiding this comment

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

How about valid options are... instead of valid are...

' or "python-fwf")' % engine)
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 use .format(...)

self._engine = klass(self.f, **self.options)

def _failover_to_python(self):
Expand Down
12 changes: 12 additions & 0 deletions pandas/tests/io/parser/test_parsers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

import os

import pytest

import pandas.util.testing as tm

from pandas import read_csv, read_table
Expand Down Expand Up @@ -99,3 +101,13 @@ def read_table(self, *args, **kwds):
kwds = kwds.copy()
kwds['engine'] = self.engine
return read_table(*args, **kwds)


class TestParameterValidation(object):
Copy link
Contributor

Choose a reason for hiding this comment

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

use tm.ensure_clean() as path

Copy link
Member

@gfyoung gfyoung May 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Let's move this test into common.py (in same directory). This base test class should not be touched for organizational purposes.

Copy link
Member

@gfyoung gfyoung May 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Also, why are we writing a round trip test? This can be much simpler:

data = "a\n1"
msg = "Unknown engine"
with tm.assert_raises_regex(ValueError, msg):
  read_csv(StringIO(data), engine='pyt')  # don't use self.read_csv because that will override the engine parameter

Oh and yes, use tm.assert_raises_regex instead of the pytest.raises(...) (pandas regex error message matching is a little more compact).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

docstring for tm.assert_raises_regex says to use pytest.raises

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so okay that this will get run once for every engine, even though it's the same test?

Copy link
Member

Choose a reason for hiding this comment

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

Yikes! You're right. We changed our minds about that. Mind fixing the documentation on that in a separate commit / PR?

Copy link
Member

Choose a reason for hiding this comment

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

so okay that this will get run once for every engine, even though it's the same test?

Actually, better idea: move it to test_common.py (the directory above)

def test_unknown_engine(self):
with tm.ensure_clean() as path:
df = tm.makeDataFrame()
df.to_csv(path)
with pytest.raises(ValueError) as exc_info:
read_csv(path, engine='pyt')
exc_info.match('Unknown engine')