Skip to content

Conversation

ghost
Copy link

@ghost ghost commented Mar 12, 2019

fixes #25623

@codecov
Copy link

codecov bot commented Mar 12, 2019

Codecov Report

Merging #25686 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #25686      +/-   ##
==========================================
+ Coverage   91.29%   91.29%   +<.01%     
==========================================
  Files         173      173              
  Lines       52961    52970       +9     
==========================================
+ Hits        48350    48359       +9     
  Misses       4611     4611
Flag Coverage Δ
#multiple 89.87% <100%> (ø) ⬆️
#single 41.73% <20%> (ø) ⬆️
Impacted Files Coverage Δ
pandas/io/parsers.py 95.43% <100%> (+0.08%) ⬆️
pandas/util/testing.py 88.98% <0%> (-0.1%) ⬇️

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 9185af2...4f92607. Read the comment docs.

@codecov
Copy link

codecov bot commented Mar 12, 2019

Codecov Report

Merging #25686 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #25686      +/-   ##
==========================================
+ Coverage   91.47%   91.48%   +<.01%     
==========================================
  Files         173      173              
  Lines       52872    52881       +9     
==========================================
+ Hits        48366    48376      +10     
+ Misses       4506     4505       -1
Flag Coverage Δ
#multiple 90.04% <100%> (ø) ⬆️
#single 41.81% <20%> (-0.01%) ⬇️
Impacted Files Coverage Δ
pandas/io/parsers.py 95.67% <100%> (+0.08%) ⬆️

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 9821b77...bdea635. Read the comment docs.

@@ -18,7 +18,7 @@ including other versions of pandas.
.. _whatsnew_0242.regressions:

Fixed Regressions
^^^^^^^^^^^^^^^^^
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like some git issues. Try reverting the changes to this file.

@jreback jreback requested a review from gfyoung March 13, 2019 17:12
@jreback jreback added IO CSV read_csv, to_csv Error Reporting Incorrect or improved errors from pandas labels Mar 13, 2019
Copy link
Member

@gfyoung gfyoung left a comment

Choose a reason for hiding this comment

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

Nice work, @heckeop !

@jreback back over to you

@jreback jreback added this to the 0.25.0 milestone Mar 18, 2019
@jreback
Copy link
Contributor

jreback commented Mar 18, 2019

lgtm. @heckeop can you merge master. ping on green.

data = "a,b,c\n1,2,3\n4,5,6"
parser = all_parsers

with pytest.raises(ValueError, match=_msg_validate_usecols_names):
Copy link
Member

Choose a reason for hiding this comment

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

although this technically doesn't fail the regex, it appears that _msg_validate_usecols_names is intended to be used with .format()

Copy link
Member

Choose a reason for hiding this comment

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

Yea good catch @simonjayhawkins

Copy link
Author

Choose a reason for hiding this comment

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

indeed! @jreback I'll push an update this weekend

data = "a,b,c\n1,2,3\n4,5,6"
parser = all_parsers

with pytest.raises(ValueError, match=_msg_validate_usecols_names):
Copy link
Member

Choose a reason for hiding this comment

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

Yea good catch @simonjayhawkins

@jreback
Copy link
Contributor

jreback commented Mar 19, 2019

can you merge master. ping on green.

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 merge master and update the whatsnew. ping on green.

@@ -123,7 +123,7 @@ Bug Fixes
~~~~~~~~~
- Bug in :func:`to_datetime` which would raise an (incorrect) ``ValueError`` when called with a date far into the future and the ``format`` argument specified instead of raising ``OutOfBoundsDatetime`` (:issue:`23830`)
- Bug in an error message in :meth:`DataFrame.plot`. Improved the error message if non-numerics are passed to :meth:`DataFrame.plot` (:issue:`25481`)
-
- Bug in ``read_csv`` which would not raise ``ValueError`` if a column index in ``usecols`` was out of bounds (:issue:`25623`)
Copy link
Contributor

Choose a reason for hiding this comment

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

this needs to be moved to the I/O section of bug fixes

@pep8speaks
Copy link

pep8speaks commented Mar 23, 2019

Hello @heckeop! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-03-25 02:29:50 UTC

@ghost
Copy link
Author

ghost commented Mar 23, 2019

@jreback @WillAyd thoughts?

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.

doc changes, ping on green.

@@ -182,7 +182,8 @@ Performance Improvements

Bug Fixes
~~~~~~~~~

- Bug in :func:`to_datetime` which would raise an (incorrect) ``ValueError`` when called with a date far into the future and the ``format`` argument specified instead of raising ``OutOfBoundsDatetime`` (:issue:`23830`)
Copy link
Contributor

Choose a reason for hiding this comment

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

remove these; these have already been moved

@@ -2632,6 +2644,10 @@ def _infer_columns(self):
ncols = len(line)
num_original_columns = ncols

# GH25623
Copy link
Contributor

Choose a reason for hiding this comment

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

here as well

@ghost
Copy link
Author

ghost commented Mar 30, 2019

@jreback Thoughts?

@jreback jreback merged commit 025b5dc into pandas-dev:master Mar 30, 2019
@jreback
Copy link
Contributor

jreback commented Mar 30, 2019

thanks @heckeop the test is pretty clear i think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Error Reporting Incorrect or improved errors from pandas IO CSV read_csv, to_csv
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ENH/BUG: usecols does not raise an exception when col index is out of bounds.
7 participants