Fix isoparse issue where single-digit minutes or seconds was allowed at the end of a string #1077
Conversation
As revealed in dateutilGH-1074, `isoparse()` is accepting 1-digit values for minutes and seconds at the end of ISO 8601 strings. This adds some failing tests.
Fixes error in dateutilGH-1074, where `isoparse` was accepting strings that have single-digit seconds or minutes at the end of an time string.
| ('20120411T03:3', ValueError), # HH:M is invalid | ||
| ('20120411T03:30:1', ValueError), # HH:MM:S is invalid |
There was a problem hiding this comment.
| ('20120411T03:3', ValueError), # HH:M is invalid | |
| ('20120411T03:30:1', ValueError), # HH:MM:S is invalid | |
| ('20120411T0', ValueError), # H is invalid | |
| ('20120411T03:3', ValueError), # HH:M is invalid | |
| ('20120411T03:30:1', ValueError), # HH:MM:S is invalid |
Any reason for not including the single digit hour case as well?
| @@ -354,6 +354,9 @@ def _parse_isotime(self, timestr): | |||
| if (has_sep and pos < len_str and | |||
| timestr[pos:pos + 1] == self._TIME_SEP): | |||
| pos += 1 | |||
| elif pos > len_str: | |||
| raise ValueError( | |||
| "Must specify 2-digit hours, minutes and seconds") | |||
There was a problem hiding this comment.
You mention hours in the error message, but the hour case is currently covered by L337 ValueError('ISO time too short'). It would be nice if we could remove that error, since it should be covered by this more descriptive error.
However, that error currently also covers empty time strings, as well as the test case ('2012-0425', ValueError), # Inconsistent date separators, presumably because the 2 is interpreted as a separator.
I was thinking we could have it cover only the empty time string and make the error message explicit. The inconsistent date separator case should probably also get a better error message, but that is probably another issue.
Summary of changes
This was happening because in Python, slices are allowed to go past the end of a string, and
int(b'3'[0:2]) == 3. We need to check that we didn't run past the end of the string when parsing these components.Closes #1074
Pull Request Checklist