-
-
Notifications
You must be signed in to change notification settings - Fork 32.6k
gh-69117: Stop checking for valid month and day in strptime regex #26215
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2611,6 +2611,19 @@ def test_strptime(self): | |
with self.assertRaises(ValueError): strptime("-000", "%z") | ||
with self.assertRaises(ValueError): strptime("z", "%z") | ||
|
||
def test_strptime_detailed_error(self): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A few things about these tests:
If it were me, I'd probably put these tests in a loop, like so: bad_month_re = "month must be in 1..12"
bad_day_re = "day is out of range for month"
tests = [
("0", "%m", bad_month_re),
("13", "%m", bad_month_re),
("0", "%d", bad_day_re),
("32", "%d", bad_day_re),
]
for dt_str, fmt_str, error_re in tests:
with self.subTest(dt_str, fmt_str):
with self.assertRaisesRegex(error_re):
self.theclass.strptime(dt_str, fmt_str) The use of a for loop for parameterization is something of a personal choice, though, feel free to skip that part. I find it a bit nicer because you can easily and cleanly add cases, but every control structure in a test is a possible source of error, so it's a bit of a toss-up. |
||
# bpo-24929: detailed errors for month and day parsing failure | ||
|
||
strptime = self.theclass.strptime | ||
|
||
self.assertRaisesRegex(ValueError, "month must be in 1..12", strptime, '0', '%m') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As a general rule I don't like Can we add a comment saying something like, "These tests are regression tests to ensure we have a good error message. We do not intend to guarantee any one specific error message." Edit: ...On second thought, this might be more of a problem than I originally thought, because this is actually simply propagating the error from the I'd love any suggestions on how we can test this without relying on that particular implementation detail (I am very much open to the possibility of just not putting a test for this at all). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The idea behind the original ticket was basically, "the regex is giving a really opaque error, and if we simply rely on the datetime constructor instead, we will get a much more informative error" but if the problem is "we're throwing an opaque error", maybe I can instead make a more useful error at the regex level. |
||
strptime('10', '%m') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Presumably this is already covered by another test somewhere and is not necessary here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Definitely not required - I meant for clarity line them up in a (too low, error) - (just right, no error) - (too high, error) triplet - can remove |
||
self.assertRaisesRegex(ValueError, "month must be in 1..12", strptime, '13', '%m') | ||
|
||
self.assertRaisesRegex(ValueError, "day is out of range for month", strptime, '0', '%d') | ||
strptime('10', '%d') | ||
self.assertRaisesRegex(ValueError, "day is out of range for month", strptime, '32', '%d') | ||
|
||
def test_strptime_single_digit(self): | ||
# bpo-34903: Check that single digit dates and times are allowed. | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Get more useful error messages when using strptime to parse strings with bad month or day values. |
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.
This comment seems to indicate that
%c
might break if we don't allow for a space before a single digit, so I think we actually want(?P<d>\d{2}| {0,1}\d)"
or(?P<d>\d{1,2}| \d)
.That said,
datetime.strptime(datetime(2021, 1, 1).strftime("%c"), "%c")
seems to work OK with this change, so 🤷, maybe we just need to remove this comment? Chesterton's Fence indicates that we should probably try and figure out whether something has changed between when this comment was written and now.