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 ListSeparator with ICU #44732

Merged
merged 3 commits into from
Nov 19, 2020
Merged

Fix ListSeparator with ICU #44732

merged 3 commits into from
Nov 19, 2020

Conversation

tarekgh
Copy link
Member

@tarekgh tarekgh commented Nov 16, 2020

#43795

When using ICU, we used to return the TextInfo.ListSeparator value as the decimal separator which is incorrect. The fix here is return the same values of ListSeparator which is returned by NLS. This fix will avoid the breaks when using such separator when parsing Excel CSV files containing a list of multi values data.

@ghost
Copy link

ghost commented Nov 16, 2020

Tagging subscribers to this area: @tarekgh, @safern, @krwq
See info in area-owners.md if you want to be subscribed.

Issue Details
Description:

#43795

When using ICU, we used to return the TextInfo.ListSeparator value as the decimal separator which is incorrect. The fix here is return the same values of ListSeparator which is returned by NLS. This fix will avoid the breaks when using such separator when parsing Excel CSV files containing a list of multi values data.

Author: tarekgh
Assignees: -
Labels:

area-System.Globalization

Milestone: -

@tarekgh
Copy link
Member Author

tarekgh commented Nov 16, 2020

@ericstj @danmosemsft @safern this is the fix for the List Separator issue which caused parsing CSV files to break when using ICU. I'll port this to 5.0 servicing branch as soon as I merge it in the master.

@tarekgh
Copy link
Member Author

tarekgh commented Nov 17, 2020

/backport to release/5.0

@github-actions
Copy link
Contributor

Started backporting to release/5.0: https://github.com/dotnet/runtime/actions/runs/368791361

int separator = IcuLocaleData.GetLocaleDataNumericPart(cultureName, IcuLocaleDataParts.DigitSubstitutionOrListSeparator);
if (separator != -1)
{
switch (separator & 0xFFFF0000)
Copy link
Member

Choose a reason for hiding this comment

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

Should this mask be in a named const in IcuLocaleData.cs, since it's strongly related to the values of the various XxSep named consts there?

Copy link
Member Author

Choose a reason for hiding this comment

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

I already have this PR ported to the 5.0 release. How important to apply this comment there? I can add it to master though.

Copy link
Member

Choose a reason for hiding this comment

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

master is fine. I didn't get a chance to review that PR before it was merged.

Copy link
Member Author

Choose a reason for hiding this comment

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

The servicing PR is not merged yet. It is here #44823 just in case.

Copy link
Member

Choose a reason for hiding this comment

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

The servicing PR is not merged yet. It is here #44823 just in case.

@tarekgh if you add a commit here and then run the /backport ... comment, if there is a PR open, it will just push the new commits to that PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I'll do that then. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

This is done and the delta change also included in the 5.0 PR too as @safern thankfully pointed.

Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

One comment, otherwise LGTM.

@tarekgh
Copy link
Member Author

tarekgh commented Nov 18, 2020

/backport to release/5.0

@github-actions
Copy link
Contributor

Started backporting to release/5.0: https://github.com/dotnet/runtime/actions/runs/370964110

@tarekgh tarekgh merged commit 496bc88 into dotnet:master Nov 19, 2020
@tarekgh tarekgh deleted the FixListSeparator branch November 19, 2020 03:20
@ghost ghost locked as resolved and limited conversation to collaborators Dec 19, 2020
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.

5 participants