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

More stern warning about string culture comparison #9388

Merged
merged 2 commits into from
Oct 13, 2023

Conversation

Smaug123
Copy link
Contributor

@Smaug123 Smaug123 commented Oct 13, 2023

Summary

Add a further warning about InvariantCulture in string comparison methods.

It continues to be true that you need to read lots of documentation before using String.StartsWith and friends, and the docs continue not to tell you that this is the case. This PR elevates some more of the most critical information from Best Practices for Using Strings to the API docs.

Source for "rarely appropriate" is Best Practices:

On balance, the invariant culture has few properties that make it useful for comparison. It does comparison in a linguistically relevant manner, which prevents it from guaranteeing full symbolic equivalence, but it isn't the choice for display in any culture. One of the few reasons to use StringComparison.InvariantCulture for comparison is to persist ordered data for a cross-culturally identical display. For example, if a large data file that contains a list of sorted identifiers for display accompanies an application, adding to this list would require an insertion with invariant-style sorting.

ref: #8973

@Smaug123 Smaug123 requested a review from a team as a code owner October 13, 2023 14:57
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Oct 13, 2023
@learn-build-service-prod
Copy link

Learn Build status updates of commit d256314:

✅ Validation status: passed

File Status Preview URL Details
xml/System/String.xml ✅Succeeded View

For more details, please refer to the build report.

For any questions, please:

@MSDN-WhiteKnight
Copy link
Contributor

I don't think this change is needed. These notes basically say "This overload uses current culture implicitly, but this might be unexpected. Consider instead using overload that explicitly accepts culture." Nowhere it was implied that InvariantCulture should be used. It is not useful to add that InvariantCulture is rarely appropriate, it's too vague, and too disconnected from the preceding text. If any addition is needed here, it should be more actionable, something like "If you don't need linguistic comparison, consider using StringComparison.Ordinal instead".

Copy link
Member

@tarekgh tarekgh left a comment

Choose a reason for hiding this comment

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

@Smaug123 thanks for submitting the PR and updating the note. The new suggested changes LGTM.

@learn-build-service-prod
Copy link

Learn Build status updates of commit 046e927:

✅ Validation status: passed

File Status Preview URL Details
xml/System/String.xml ✅Succeeded View

For more details, please refer to the build report.

For any questions, please:

@learn-build-service-prod
Copy link

Learn Build status updates of commit b3a3dbd:

✅ Validation status: passed

File Status Preview URL Details
xml/System/String.xml ✅Succeeded View

For more details, please refer to the build report.

For any questions, please:

@gewarren gewarren merged commit 5422621 into dotnet:main Oct 13, 2023
3 checks passed
@Smaug123 Smaug123 deleted the string-culture-warning branch March 27, 2024 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Runtime community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants