Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.7k
gh-69714: Makecalendar module fully tested#93655
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…}Calendar(locale='')`
….Locale{Text|HTML}Calendar`There are 3 paths to use `locale` argument in`calendar.Locale{Text|HTML}Calendar.__init__(..., locale=None)`:(1) `locale=None` -- denotes the "default locale"[1](2) `locale=""` -- denotes the native environment(3) `locale=other_valid_locale` -- denotes a custom localeSo far case (2) is covered and case (1) is in78935da (same branch).This commit adds a remaining case (3).[1] In the current implementation, this translates into the followingapproach:GET current localeIF current locale == "C" THEN SET current locale TO "" GET current localeENDIFThis condition cannot be true. `_locale.setlocale()` from the C moduleraises `locale.Error` instead of returning `None` for`different_locale.__enter__` (where `self.oldlocale` is set).
This patch has already been applied to `main` branch (viapythongh-93468), butwith wrong copyright. After merging this commit to `main`, git `Author`metadata will be updated to the original author.
ghost commentedJun 9, 2022 • edited by ghost
Loading Uh oh!
There was an error while loading.Please reload this page.
edited by ghost
Uh oh!
There was an error while loading.Please reload this page.
The following commit authors need to sign the Contributor License Agreement: |
bedevere-bot commentedJun 9, 2022
Most changes to Pythonrequire a NEWS entry. Please add it using theblurb_it web app or theblurb command-line tool. |
AA-Turner left a comment
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.
Also cc:@hugovk (who I know has different opinions on testing to me!)
A
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
bedevere-bot commentedJun 9, 2022
Most changes to Pythonrequire a NEWS entry. Please add it using theblurb_it web app or theblurb command-line tool. |
AA-Turner commentedJun 9, 2022
You will need a NEWS entry, along the lines of A |
bxsx commentedJun 9, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Thanks! I just added it. Another problem is the lack of CLA signatures from@jesstess and@rohitmediratta. The applied patches were quite old, so I'm not sure the email addresses are up to date (I don't know if the bot is sending emails about the CLA). Anyway, it looks like GitHub has assigned these commits to the correct usernames, so I'm tagging Jessica and Rohit in this comment to help notify :) |
AA-Turner commentedJun 9, 2022
You'll need to fix the Ubuntu failures in Traceback (most recent call last): File"/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/test_calendar.py", line650, intest_locale_calendar_formatmonthnameself.assertEqual(cal.formatmonthname(2022,6,2,withyear=False),"June")^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File"/home/runner/work/cpython/cpython-ro-srcdir/Lib/calendar.py", line588, informatmonthnamewith different_locale(self.locale):^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File"/home/runner/work/cpython/cpython-ro-srcdir/Lib/calendar.py", line555, in__enter__ _locale.setlocale(_locale.LC_TIME,self.locale)^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File"/home/runner/work/cpython/cpython-ro-srcdir/Lib/locale.py", line626, insetlocalereturn _setlocale(category, locale)^^^^^^^^^^^^^^^^^^^^^^^^^^^^locale.Error:unsupported locale setting This may be another test-skipping scenario, I don't know what the values it is trying to set are though so I can't guess much further. A |
AA-Turner commentedJun 9, 2022
You might also want to consider emailing the two of them if you don't hear back, neither seem to have much recent publicly shown activity on GH. A |
bxsx commentedJun 9, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Or maybe (based on the traceback) it's related to one of the corner cases from#93655 (comment)? |
…verage)"This reverts commita96786c.
AA-Turner commentedJun 9, 2022
Sadly it doesn't seem to have worked :( A |
…()`.This method temporarily changes the current locale to the given locale,so `_locale.setlocale()` may raise `local.Error`.
bxsx commentedJun 10, 2022
Yes, I misread the traceback in a rush before disconnecting, but I wanted to kick off builds to get results when I get back. Time saving ;) For now, I have restoreda96786c. However, it is true that the cause of the problem is related to#31214 and as you correctly predicted, this is anothertest-skipping scenario. Fixed in2b6f1f9. |
bxsx commentedJun 10, 2022
|
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
No need to process the function.This also helps in supporting CLI testing.
These tests are now part of `test_illegal_arguments`.This reverts commit238c527.
bxsx commentedJun 11, 2022
Fixed in8b7bc06. |
bxsx commentedJun 11, 2022
Ok, all comments are now addressed and resolved. All tests pass successfully. ✔️ I think the PR is ready for another round of reviews ;-) |
AA-Turner left a comment
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.
The updated tests look good, I'll wait for Hugo's thoughts.
A
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
bxsx commentedJun 17, 2022
Good news for the weekend. Rohit signed the CLA. Thanks again@rohitmediratta! I also looked at@jesstess GH profile and I believe the missing CLA has already been signed by Jessica as she is aformer director of the Python Software Foundation and a member of@python. I've emailedcontributions@python.org to sort this out. |
terryjreedy commentedFeb 24, 2023
Here is the holdup situation: When moved to gh, issue became#57539. Irit Katriel requested gh PR and mentioned this issue. On June 9, bxsx addedab069a4, which added Jessica's original code, without the Serhiy correction. It somehow credited Jessica as author. Fortunately, the deletion of the correction is somehow not in the merged patch. Unfortunately, it fooled the CLA bot into thinking that she was an author of this PR. @ambv This PR, which has had a lot of work by multiple people, is blocked by the false 'need' for an unobtainable signature from@jesstess Jessica McKeller, who did not contribute to this PR. Should reverting ab069a4 fix the mistake or is something else needed. If so, can you override the bot? (Though a final review may be needed.) |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Summary
Resolves#69714 by increasing test coverage of
calendarmodule to100%[1].Coverage reports
Before
mainbranch test coverage report forcalendarmodule:After
The PR branch test coverage report for
calendarmodule:Footnotes
[1] The remaining three lines are:
and they are,in fact, already covered by tests (only not reported by coveragepy).