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

[API Proposal]: System.Globalization.CompareOptions.UseNLS #99646

Closed
clement911 opened this issue Mar 12, 2024 · 9 comments
Closed

[API Proposal]: System.Globalization.CompareOptions.UseNLS #99646

clement911 opened this issue Mar 12, 2024 · 9 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Globalization

Comments

@clement911
Copy link

Background and motivation

Using NLS instead of ICU can currently only be configured with a global flag.
See https://learn.microsoft.com/en-us/dotnet/core/extensions/globalization-icu#use-nls-instead-of-icu
This new API would allow the use of NLS on a per API call basis.

API Proposal

Add a new flag System.Globalization.CompareOptions.UseNLS

API Usage

CompareInfo.GetCompareInfo("en-US").Compare(str1, str2, CompareOptions.UseNLS)

Alternative Designs

I can't think of any

Risks

I can't think of any

@clement911 clement911 added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Mar 12, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-globalization
See info in area-owners.md if you want to be subscribed.

@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Mar 12, 2024
@teo-tsirpanis
Copy link
Contributor

I don't think we should be encouraging users to rely on NLS in the long term, especially for new apps. NLS is Windows-specific while ICU is standard and cross-platform.

@clement911
Copy link
Author

Not encourage but it is sometimes needed to use NLS.
See #99507

@teo-tsirpanis
Copy link
Contributor

SqlString should call NLS APIs by itself. I don't see a reason to expose a general-purpose API for NLS comparisons.

@clement911
Copy link
Author

SqlString delegates to System.Globalization.CompareOptions for its comparison logic.

See:

public static CompareOptions CompareOptionsFromSqlCompareOptions(SqlCompareOptions compareOptions)
{
CompareOptions options = CompareOptions.None;
ValidateSqlCompareOptions(compareOptions);
if ((compareOptions & (SqlCompareOptions.BinarySort | SqlCompareOptions.BinarySort2)) != 0)
throw ADP.ArgumentOutOfRange(nameof(compareOptions));
else
{
if ((compareOptions & SqlCompareOptions.IgnoreCase) != 0)
options |= CompareOptions.IgnoreCase;
if ((compareOptions & SqlCompareOptions.IgnoreNonSpace) != 0)
options |= CompareOptions.IgnoreNonSpace;
if ((compareOptions & SqlCompareOptions.IgnoreKanaType) != 0)
options |= CompareOptions.IgnoreKanaType;
if ((compareOptions & SqlCompareOptions.IgnoreWidth) != 0)
options |= CompareOptions.IgnoreWidth;
}
return options;
}

CompareOptions options = CompareOptionsFromSqlCompareOptions(x.m_flag);
iCmpResult = x.m_cmpInfo!.Compare(x.m_value, 0, cwchX, y.m_value, 0, cwchY, options);

I guess it could be a new internal API, although I don't see the harm of exposing this flag since it would be off by default.
Also, there might be other public packages that might want to use it.

@MichalPetryka
Copy link
Contributor

SqlString should call NLS APIs by itself. I don't see a reason to expose a general-purpose API for NLS comparisons.

That probably won't happen since it'd make SqlString Windows only.

@jkotas
Copy link
Member

jkotas commented Mar 13, 2024

Globalization tables are moving target. If SqlString assumes that the globalization tables are fixed, it sounds like a design flaw in SqlString that should be fixed in SqlString.

@clement911
Copy link
Author

Any updates on this?
We continue to find more characters that are not handled properly by SqlString (see #99507 (comment))

Can we loop in someone that owns SqlString, or knows more about it?

@jkotas
Copy link
Member

jkotas commented May 22, 2024

The main issue is tracked by #99507

@jkotas jkotas closed this as completed May 22, 2024
@dotnet-policy-service dotnet-policy-service bot removed the untriaged New issue has not been triaged by the area owner label May 22, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jun 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Globalization
Projects
None yet
Development

No branches or pull requests

4 participants