Skip to content

gh-137481: consider actual day name length #137482

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

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Conversation

snoyes
Copy link
Contributor

@snoyes snoyes commented Aug 6, 2025

Find the length of the longest day name in the current locale. Use that length, rather than a constant "9", to decide if the names should be abbreviated.

Find the length of the longest day name in the current locale. Use that length, rather than a constant "9", to decide if the names should be abbreviated.
Copy link
Member

@AA-Turner AA-Turner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please can you add a test (test_calendar.py) and a NEWS entry?

@bedevere-app
Copy link

bedevere-app bot commented Aug 6, 2025

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@snoyes
Copy link
Contributor Author

snoyes commented Aug 6, 2025

I have made the requested changes; please review again.

@bedevere-app
Copy link

bedevere-app bot commented Aug 6, 2025

Thanks for making the requested changes!

@AA-Turner: please review the changes made to this pull request.

@bedevere-app bedevere-app bot requested a review from AA-Turner August 6, 2025 21:58
@snoyes
Copy link
Contributor Author

snoyes commented Aug 7, 2025

Please advise on the test - I can provide a few specific locales to test, such as Malay, Danish, and French, which illustrates the problem with the current code but might be too specific and so miss out on some other defect. Or I can loop through all available locales as in the latest commit, which is more comprehensive but increases the time to run this one test from a few hundredths of a second to over half a second. Is that a problem?

@serhiy-storchaka
Copy link
Member

I prefer testing on a small set of example locales. I suspect that most locales are not affected by this change. Anyway, there is no portable way to get the list of all locales (locale.locale_alias is a bad substitute, and it contains many duplicates).

You can use the support.run_with_locale() or support.run_with_locales() decorators to run the test with different locales (the former runs with the first supported locale, the latter runs with all supported locales from the list).

@snoyes
Copy link
Contributor Author

snoyes commented Aug 8, 2025

It's not most locales, but it's more than a dozen, including Chinese, Danish, French, Japanese, Korean, Malay, Norwegian, and Thai.

The same bug also produces the opposite error: if any weekday names are longer than 9 characters, then a calendar width of 9 simply truncates rather than using the proper abbreviation. That includes Bulgarian, Croatian, Finnish, German, Icelandic, Lithuanian, Polish, Portuguese, Russian, Sinhala, and Slovenian.

So the only locales not affected are those which happen to have exactly 9 characters in their longest weekday name.

I've changed the test case to use support.run_with_locale() and provided a short list, and added a second test case for the locales which ought to be properly abbreviated even if the supplied width exceeds 9 characters.

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your update.

Some lines are too long, please ensure that all lines do not exceed 79 columns, as required by PEP 8. If you give a name to lambda, using a local function is better.

Weekday names may be platform specific, so on particular platform the first found locale may have names not suitable for the test. I suggest to use run_with_locales() and skip the subtest for non-suitable locale. For example:

if max_length >= 9:
    self.skipTest('weekday names are too long')

@snoyes
Copy link
Contributor Author

snoyes commented Aug 8, 2025

I have made the requested changes; please review again.

@bedevere-app
Copy link

bedevere-app bot commented Aug 8, 2025

Thanks for making the requested changes!

@AA-Turner: please review the changes made to this pull request.

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. 👍

@serhiy-storchaka
Copy link
Member

There are trailing whitespaces.

@snoyes
Copy link
Contributor Author

snoyes commented Aug 8, 2025

Trailing whitespaces removed. I have made the requested changes; please review again.

@bedevere-app
Copy link

bedevere-app bot commented Aug 8, 2025

Thanks for making the requested changes!

@AA-Turner, @serhiy-storchaka: please review the changes made to this pull request.

@bedevere-app bedevere-app bot requested a review from serhiy-storchaka August 8, 2025 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants