Skip to content
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

Fix Time Zone Id Conversion with Lowercased Regions #53315

Merged

Conversation

tarekgh
Copy link
Member

@tarekgh tarekgh commented May 26, 2021

Fixes #53279

In the TimeZoneInfo.TryConvertWindowsIdToIanaId we use ICU to convert the Windows time zone Ids to IANA Ids. One of the parameters used in this API is the region name. Unfortunately, ICU always expect the region name to be uppercased and return unexpected results if passed lowercased region names. This change is to ensure we pass uppercased region names to ICU.
The change here manually uppercasing the region names to avoid extra string allocation as the valid region names are short anyway and will have all character in ASCII range.

@ghost
Copy link

ghost commented May 26, 2021

Tagging subscribers to this area: @tannergooding
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #53279

In the TimeZoneInfo.TryConvertWindowsIdToIanaId we use ICU to convert the Windows time zone Ids to IANA Ids. One of the parameters used in this API is the region name. Unfortunately, ICU always expect the region name to be uppercased and return unexpected results if passed lowercased region names. This change is to ensure we pass uppercased region names to ICU.
The change here manually uppercasing the region names to avoid extra string allocation as the valid region names are short anyway and will have all character in ASCII range.

Author: tarekgh
Assignees: -
Labels:

area-System.Runtime

Milestone: -

@tarekgh tarekgh added this to the 6.0.0 milestone May 26, 2021
@tarekgh
Copy link
Member Author

tarekgh commented May 27, 2021

The failures in the CI looks tracked by the issue #53311

@danmoseley
Copy link
Member

valid region names are short anyway and will have all character in ASCII range.

Is ASCII required in a standard somewhere or just in practice?

@tarekgh
Copy link
Member Author

tarekgh commented May 27, 2021

Is ASCII required in a standard somewhere or just in practice?

It is Latin alpha characters which is technically is the ASCII alpha characters. Also, it can be numbers for countries which don't use Latin scripts.

ISO 3166

The country codes can be represented either as a two-letter code (alpha-2) which is recommended as the general-purpose code, a three-letter code (alpha-3) which is more closely related to the country name and a three-digit numeric code (numeric-3) which can be useful if you need to avoid using Latin script.

The Wikipedia table is useful to see all assigned codes and which not assigned.

Co-authored-by: Martin Costello <martin@martincostello.com>
@tarekgh
Copy link
Member Author

tarekgh commented May 28, 2021

@stephentoub could you please have a look and approve it if it looks ok to you? Thanks!

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

sorry - hit the wrong button, but I can't delete this.

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

:shipit:

@stephentoub
Copy link
Member

Requested changes must have been addressed really quickly 😄

image

@ghost
Copy link

ghost commented May 28, 2021

Hello @stephentoub!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@tarekgh
Copy link
Member Author

tarekgh commented May 28, 2021

The failures in the following CI are unrelated and tracked by the issues #53311, #52843, #45568

@tarekgh tarekgh merged commit e3f0144 into dotnet:main May 28, 2021
@tarekgh tarekgh deleted the FixTimeZoneIdConversionWithLowercasedRegions branch May 28, 2021 17:46
@ghost ghost locked as resolved and limited conversation to collaborators Jun 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TimeZoneInfo.TryConvertWindowsIdToIanaId() difference compared to TZConvert.TryWindowsToIana()
7 participants