Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-124529: Fix _strptime to make %c/%x accept a year with fewer digits#124778
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?
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
827db451d916a0a4ba3c9cef07226556bdf56c84dcb42783c97b523e6c2addffb32c03722efe47b46a483d4116a8eac1061956662a77c3dab3b161f949e66a059d4c595753aced4582b990b075ecc359d6d690ca4ad1062File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -161,11 +161,42 @@ def test_compile(self): | ||
| for directive in ('a','A','b','B','c','d','G','H','I','j','m','M','p', | ||
| 'S','u','U','V','w','W','x','X','y','Y','Z','%'): | ||
| fmt = "%d %Y" if directive == 'd' else "%" + directive | ||
| input_string = time.strftime(fmt) | ||
| compiled = self.time_re.compile(fmt) | ||
| found = compiled.match(time.strftime(fmt)) | ||
| self.assertTrue(found, "Matching failed on '%s' using '%s' regex" % | ||
| (time.strftime(fmt), | ||
| compiled.pattern)) | ||
| found = compiled.match(input_string) | ||
| self.assertTrue(found, | ||
| (f"Matching failed on '{input_string}' " | ||
| f"using '{compiled.pattern}' regex")) | ||
| for directive in ('c', 'x'): | ||
| # gh-124529 | ||
| fmt = "%" + directive | ||
| with self.subTest(f"{fmt!r} should match input containing " | ||
| f"year with fewer digits than usual"): | ||
| try: | ||
| (input_string, | ||
| _) = _get_data_to_test_strptime_with_few_digits_year(fmt) | ||
| except AssertionError as exc: | ||
| self.fail(str(exc)) | ||
| compiled = self.time_re.compile(fmt) | ||
| found = compiled.match(input_string) | ||
| self.assertTrue(found, | ||
| (f"Matching failed on '{input_string}' " | ||
| f"using '{compiled.pattern}' regex")) | ||
| for directive in ('y', 'Y', 'G'): | ||
| fmt = "%" + directive | ||
| with self.subTest(f"{fmt!r} should not match input containing " | ||
| f"year with fewer digits than usual"): | ||
| try: | ||
| (input_string, | ||
| _) = _get_data_to_test_strptime_with_few_digits_year(fmt) | ||
| except AssertionError as exc: | ||
| self.fail(str(exc)) | ||
| compiled = self.time_re.compile(fmt) | ||
| found = compiled.match(input_string) | ||
| self.assertFalse(found, | ||
| (f"Matching unexpectedly succeeded " | ||
| f"on '{input_string}' using " | ||
| f"'{compiled.pattern}' regex")) | ||
| def test_blankpattern(self): | ||
| # Make sure when tuple or something has no values no regex is generated. | ||
| @@ -299,6 +330,28 @@ def helper(self, directive, position): | ||
| (directive, strf_output, strp_output[position], | ||
| self.time_tuple[position])) | ||
| def helper_for_directives_accepting_few_digits_year(self, directive): | ||
| fmt = "%" + directive | ||
| try: | ||
| (input_string, | ||
Contributor 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. Hmmm, Wouldn't this be weird? ContributorAuthor 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. Why? IMHO, OK. | ||
| expected_year, | ||
| ) = _get_data_to_test_strptime_with_few_digits_year(fmt) | ||
| except AssertionError as exc: | ||
| self.fail(str(exc)) | ||
| try: | ||
| output_year = _strptime._strptime(input_string, fmt)[0][0] | ||
Contributor 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. Is't possible for ContributorAuthor 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. It always returns a 3- | ||
| except ValueError as exc: | ||
| # See: gh-124529 | ||
| self.fail(f"testing of{directive!r} directive failed; " | ||
| f"{input_string!r} -> exception:{exc!r}") | ||
| else: | ||
| self.assertEqual(output_year, expected_year, | ||
| (f"testing of{directive!r} directive failed; " | ||
| f"{input_string!r} -> output including year " | ||
| f"{output_year!r} !={expected_year!r}")) | ||
| def test_year(self): | ||
| # Test that the year is handled properly | ||
| for directive in ('y', 'Y'): | ||
| @@ -312,6 +365,17 @@ def test_year(self): | ||
| "'y' test failed; passed in '%s' " | ||
| "and returned '%s'" % (bound, strp_output[0])) | ||
| def test_bad_year(self): | ||
| for directive, bad_inputs in ( | ||
| ('y', ('9', '100', 'ni')), | ||
| ('Y', ('7', '42', '999', '10000', 'SPAM')), | ||
| ): | ||
| fmt = "%" + directive | ||
Contributor 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. Maybe you can use f-string. ContributorAuthor 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. Both ways are valid; the one I chose is more consistent with the existing code in this module. | ||
| for input_val in bad_inputs: | ||
| with self.subTest(directive=directive, input_val=input_val): | ||
| with self.assertRaises(ValueError): | ||
| _strptime._strptime_time(input_val, fmt) | ||
| def test_month(self): | ||
| # Test for month directives | ||
| for directive in ('B', 'b', 'm'): | ||
| @@ -454,11 +518,21 @@ def test_date_time(self): | ||
| for position in range(6): | ||
| self.helper('c', position) | ||
| def test_date_time_accepting_few_digits_year(self): # gh-124529 | ||
| # Test %c directive with input containing year | ||
| # number consisting of fewer digits than usual | ||
| self.helper_for_directives_accepting_few_digits_year('c') | ||
| def test_date(self): | ||
| # Test %x directive | ||
| for position in range(0,3): | ||
| self.helper('x', position) | ||
| def test_date_accepting_few_digits_year(self): # gh-124529 | ||
| # Test %x directive with input containing year | ||
| # number consisting of fewer digits than usual | ||
| self.helper_for_directives_accepting_few_digits_year('x') | ||
| def test_time(self): | ||
| # Test %X directive | ||
| for position in range(3,6): | ||
| @@ -769,5 +843,59 @@ def test_TimeRE_recreation_timezone(self): | ||
| _strptime._strptime_time(oldtzname[1], '%Z') | ||
| def _get_data_to_test_strptime_with_few_digits_year(fmt): | ||
| # This helper, for the given format string (fmt), returns a 2-tuple: | ||
| # (<strptime input string>, <expected year>) | ||
| # where: | ||
| # * <strptime input string> -- is a `strftime(fmt)`-result-like str | ||
| # containing a year number which is *shorter* than the usual four | ||
| # or two digits, because the number is small and *not* 0-padded | ||
| # (namely: here the contained year number consist of one digit: 7 | ||
| # -- that's an arbitrary choice); | ||
| # * <expected year> -- is an int representing the year number that | ||
| # is expected to be part of the result of a `strptime(<strptime | ||
| # input string>, fmt)` call (namely: either 7 or 2007, depending | ||
| # on the given format string and current locale...). | ||
| # Note: AssertionError (with an appropriate failure message) is | ||
| # raised if <strptime input string> does *not* contain the year | ||
| # part (for the given format string and current locale). | ||
| # 1. Prepare auxiliary sample time data (note that the magic values | ||
| # we use here are guaranteed to be compatible with `time.strftime()`, | ||
| # and are also intended to be well distinguishable within formatted | ||
| # strings, thanks to the fact that the amount of overloaded numbers | ||
| # is minimized, as in `_strptime.LocaleTime.__calc_date_time()`): | ||
| sample_year = 1999 | ||
| sample_tt = (sample_year, 3, 17, 22, 44, 55, 2, 76, 0) | ||
| sample_string = time.strftime(fmt, sample_tt) | ||
| sample_year_4digits = str(sample_year) | ||
| sample_year_2digits = str(sample_year)[-2:] | ||
| # 2. Pick an arbitrary year number representation that | ||
| # is always *shorter* than the usual four or two digits: | ||
| year_1digit = '7' | ||
| # 3. Obtain the resultant 2-tuple: | ||
| if sample_year_4digits in sample_string: | ||
| input_string = sample_string.replace(sample_year_4digits, year_1digit) | ||
| if sample_year_2digits in input_string: | ||
| raise RuntimeError(f"the{fmt!r} format is not supported by " | ||
| f"this helper (a{fmt!r}-formatted string, " | ||
| f"{sample_string!r}, seems to include both a " | ||
| f"2-digit and 4-digit year number)") | ||
| expected_year = int(year_1digit) | ||
| elif sample_year_2digits in sample_string: | ||
| input_string = sample_string.replace(sample_year_2digits, year_1digit) | ||
| expected_year = 2000 + int(year_1digit) | ||
| else: | ||
| raise AssertionError(f"time.strftime({fmt!r}, ...)={sample_string!r} " | ||
| f"does not include the year{sample_year!r} in " | ||
| f"any expected format (are{fmt!r}-formatted " | ||
| f"strings supposed to include a year number? " | ||
| f"if they are, isn't there something severely " | ||
| f"wrong with the current locale?)") | ||
| return input_string, expected_year | ||
| if __name__ == '__main__': | ||
| unittest.main() | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| Fix :meth:`datetime.datetime.strptime`, :meth:`datetime.date.strptime` | ||
Contributor 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. I think you should write this This will be more concise and intuitive. ContributorAuthor 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. OK, I've shortened it; yet I'd prefer to keep the information that the fix prevents the error for certain cases of a | ||
| and :func:`time.strptime` to make ``%c`` and ``%x`` accept year numbers | ||
| with fewer digits than the usual 4 or 2 (not zero-padded). This prevents | ||
| :exc:`ValueError` in certain cases of ``strftime/strptime`` round trips. | ||
| Patch by Jan Kaliszewski. | ||
Contributor 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. Maybe you should remove the author, because this PR will not be deleted after merging. ContributorAuthor 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. Isn't adding such information a common and documented practice? Contributor 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. In the user-oriented update record, users do not care who implemented it. ContributorAuthor 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. Most users don't care, indeed; but for authors the presence of such information is a symbolic reward for the contribution. I find this custom nice. And anyway, as I wrote above, we are talking about a common practice. I am not the right person to ask about changing established practices regarding Python development. :) | ||
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.
as far as i know we use next construction
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.
and I think you could mark many comments and suggestions as resolved, otherwise they can get in the way of reading and seem ignored :)
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 it be done by the reviewer who asked a question?
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.
I think you can mark as solved.
at least I can't see this button as the author of suggestion,
so I think only you can close them, especially if there are no plans to continue this topic