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.

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

ERR: Raise the correct error for to_datetime() #23969

Closed
wants to merge 8 commits into from
Closed

ERR: Raise the correct error for to_datetime() #23969

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d9c1918
Raising correct exceptions for to_datetime() #23830
varadgunjal Nov 28, 2018
f6c1efa
Adding test case for checking that the correct exception is raised by…
varadgunjal Nov 28, 2018
cefb1c1
Fixes to correct caught in CI
varadgunjal Nov 29, 2018
b3fadc6
Checking the error with match inside pytest
varadgunjal Nov 29, 2018
399beca
Added a whatsnew note
varadgunjal Dec 1, 2018
d7adb56
Merge branch 'master' into bugfix-23830
varadgunjal Dec 5, 2018
8d8b893
Fixing issues caught in CI
varadgunjal Dec 8, 2018
6aa0bf3
Merge branch 'master' into bugfix-23830
varadgunjal Dec 24, 2018
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
4 changes: 1 addition & 3 deletions pandas/_libs/tslib.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -691,9 +691,7 @@ cpdef array_to_datetime(ndarray[object] values, errors='raise',
continue
elif require_iso8601:
if is_raise:
raise ValueError("time data {val} doesn't "
"match format specified"
.format(val=val))
continue
Copy link
Member

Choose a reason for hiding this comment

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

Don't you mean to do raise here?

Copy link
Member

@gfyoung gfyoung Nov 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Also, don't you have to make a similar change here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I take by [here] you meant line 626 in tslib.pyx?

Copy link
Member

Choose a reason for hiding this comment

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

Yep, sorry. I forgot to add the link.

return values, tz_out
raise
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree this is legit. can you add a comment here about this, e.g. its OOB (even though its just a few lines above)


Expand Down
3 changes: 3 additions & 0 deletions pandas/tests/indexes/datetimes/test_tools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -947,6 +947,9 @@ def test_to_datetime_barely_out_of_bounds(self):
with pytest.raises(OutOfBoundsDatetime):
to_datetime(arr)

with pytest.raises(OutOfBoundsDatetime):
to_datetime(arr, format="%Y-%m-%d %H:%M:%S.%f")

Copy link
Member

Choose a reason for hiding this comment

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

Move this out as a separate test and reference the issue number.

Also, check the error here with the match parameter in 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.

Thanks for the feedback. I'll move it out. However, my main issue here was that this test would pass locally but fail on the CI build (https://travis-ci.org/pandas-dev/pandas/jobs/460708541). My best guess was that it was due to tslib.pyx not being recompiled. Any ideas?

Copy link
Member

Choose a reason for hiding this comment

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

Your build may not have had this added test, as I'm relatively certain that your change above was not correct.

@pytest.mark.parametrize('cache', [True, False])
def test_to_datetime_iso8601(self, cache):
result = to_datetime(["2012-01-01 00:00:00"], cache=cache)
Expand Down