Skip to content

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

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

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 3 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
8 changes: 2 additions & 6 deletions pandas/_libs/tslib.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -623,9 +623,7 @@ cpdef array_to_datetime(ndarray[object] values, errors='raise',
iresult[i] = NPY_NAT
continue
elif is_raise:
raise ValueError("time data {val} doesn't match "
"format specified"
.format(val=val))
raise
return values, tz_out

try:
Expand Down Expand Up @@ -691,9 +689,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))
raise
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
8 changes: 8 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,14 @@ def test_to_datetime_barely_out_of_bounds(self):
with pytest.raises(OutOfBoundsDatetime):
to_datetime(arr)

def test_to_datetime(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect that we are not properly catching this time data specification mismatch at all. can you see where else we are trying to catch this (prob just put a print statement or something in the cython code to see what tests hit it).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I'm still adding some changes to it. Will notify when ready.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jreback looks like you're right. I don't see any other tests that hit that piece of the cython code.

# GH#23830 raise the correct exception when
# the format argument is passed.
arr = np.array(['2262-04-11 23:47:16.854775808'], dtype=object)

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