[release/5.0] Fix ListSeparator with ICU #44823
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
safern
approved these changes
Nov 17, 2020
|
Thanks. Did you hear back from the Office team? |
Yes, I confirmed Office is calling NLS on Windows and on Mac they carry the data same as we are doing in this PR.
Thanks. sorry I missed it in the first place :-) |
1062c91 to
ec39b92
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #43795
Backport of #44732 to release/5.0
/cc @tarekgh
Customer Impact
.NET has the property
TextInfo.ListSeparatorwhich we discovered the users take dependencies on this property to parse Excel CSV files. In .NET Core 1.0 when started to support Linux (by using ICU library), ICU doesn't provide data for the list separator. The implementers decided to use the Decimal Separator for that. When we switched to use ICU on Windows in .NET 5.0, some users started to see the difference of the data between NLS and what we return when using ICU.The fallback to Decimal Separator was wrong in the first place and also we need to maintain the same data provided by NLS (as ICU doesn't provide similar data anyway). That will return back the old behavior and return more reasonable values for the separator.
This change will apply to both Windows and Unix: it will change the behavior we have had in Unix for some time to match the NLS behavior.
I have followed up with Office team too and they verified the data we provide (which we picked from NLS) is compatible with the data they use in Excel. They also carry this same data on Mac.
Testing
I have manually confirmed the data returned from all cultures when using NLS is exact as when we use ICU now. Also, I have added a new tests for all major cultures to ensure the behavior moving forward.
Risk
I believe the risk is small as we are just adding some more hardcoded data to a table we already maintain. Also, I didn't even increase the data size as I was able to squash it inside some other fields.