-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
ERR non-ISO formats don't show position of error #50366
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.
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 non-ISO formats don't show position of error #50366
Conversation
464aa84
to
a2cff0b
Compare
a2cff0b
to
303a648
Compare
pandas/_libs/tslibs/strptime.pyx
Outdated
except (ValueError, OutOfBoundsDatetime) as ex: | ||
ex.args = (str(ex) + f" at position {i}", ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similar to
Lines 789 to 790 in 2f54a47
except (ValueError, OverflowError) as ex: | |
ex.args = (f"{ex} present at position {i}", ) |
I just removed the word 'present'
as I don't think sounds good with the errors from this file
i.e.
time data ' ' does not match format '%m/%d/%Y' \(match\) at position 2
sounds better than
time data ' ' does not match format '%m/%d/%Y' \(match\) present at position 2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit but why not use a full f-string instead of addition between 2 strings here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So IIUC the message is a bit different between ISO / non-ISO (the former has the word present?). Can we align the messages?
pandas/_libs/tslibs/strptime.pyx
Outdated
except (ValueError, OutOfBoundsDatetime) as ex: | ||
ex.args = (str(ex) + f" at position {i}", ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit but why not use a full f-string instead of addition between 2 strings here?
) | ||
with pytest.raises(OutOfBoundsDatetime, match=msg): | ||
with tm.assert_produces_warning(warning, match="Could not infer format"): | ||
to_datetime("2417-10-27 00:00:00", format=format) | ||
to_datetime("2417-10-10 00:00:00", format=format) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
slightly changing the input here so we can parametrise of it with both "%Y-%m-%d %H:%M:%S"
and "%Y-%d-%m %H:%M:%S"
(to check ISO vs non-ISO)
I've aligned them now, thanks - this should make the diff in #50242 much smaller In [1]: to_datetime(['2020-01-01', '9999-01-01'], format='%Y-%m-%d')
---------------------------------------------------------------------------
OutOfBoundsDatetime: Out of bounds nanosecond timestamp: 9999-01-01 00:00:00 present at position 1
In [2]: to_datetime(['2020-01-01', '9999-01-01'], format='%Y-%d-%m')
---------------------------------------------------------------------------
OutOfBoundsDatetime: Out of bounds nanosecond timestamp: 9999-01-01 00:00:00 present at position 1
In [3]: to_datetime(['2020-01-01', 'foo'], format='%Y-%m-%d')
---------------------------------------------------------------------------
ValueError: time data "foo" at position 1 doesn't match format "%Y-%m-%d" (match)
In [4]: to_datetime(['2020-01-01', 'foo'], format='%Y-%d-%m')
----------------------------------------------------------------------------
ValueError: time data "foo" at position 1 doesn't match format "%Y-%d-%m" (match) |
@@ -1133,10 +1139,10 @@ def test_datetime_invalid_scalar(self, value, format, warning): | |||
assert res is NaT | |||
|
|||
msg = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick can you use msg = "|".join(...)
instead of a single string
pandas/_libs/tslib.pyx
Outdated
raise ValueError( | ||
f"time data \"{val}\" at position {i} doesn't " | ||
f"match format \"{format}\"" | ||
f"match format \"{format}\" ({match_msg})" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ending this with "search" seems weird (and i dont see it in any test cases)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's true that it wasn't tested, but it's been like this since pandas 0.24:
In [1]: import pandas
In [2]: pandas.__version__
Out[2]: '0.24.0'
In [3]: pandas.to_datetime(['2020-01-f00'], format='%Y-%d-%m', exact=False)
---------------------------------------------------------------------------
ValueError: time data '2020-01-f00' does not match format '%Y-%d-%m' (search)
Maybe we can just remove it - people typically know whether they've passed exact
, and it doesn't really add much
Especially as it was never there for (more common) ISO formats:
In [4]: pandas.to_datetime(['2020-01-f00'], format='%Y-%m-%d', exact=False)
---------------------------------------------------------------------------
ValueError: time data 2020-01-f00 doesn't match format specified
Let's just unify them to
time data '2020-01-f00' does not match format '%Y-%d-%m'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks pretty good
pandas/_libs/tslibs/strptime.pyx
Outdated
@@ -396,7 +401,15 @@ def array_strptime( | |||
|
|||
result_timezone[i] = tz | |||
|
|||
except (ValueError, OutOfBoundsDatetime): | |||
except OutOfBoundsDatetime as ex: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than breaking off this branch can we not keep the existing one and just specialize for the OOB exception within?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're right, that's better - thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome thanks @MarcoGorelli
Looks like a merge conflict but feel free to merge after that |
cool, thanks for reviewing! |
@@ -1882,7 +1891,10 @@ def test_dataframe_mixed(self, cache): | |||
def test_dataframe_float(self, cache): | |||
# float | |||
df = DataFrame({"year": [2000, 2001], "month": [1.5, 1], "day": [1, 1]}) | |||
msg = "cannot assemble the datetimes: unconverted data remains: 1" | |||
msg = ( | |||
r"^cannot assemble the datetimes: unconverted data remains at position " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NBD but is the "r" necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't be, it's just a habit to always use that in regular expressions
doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.