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

String IgnoreCase extension method #30626

Open
BluePositive opened this issue Aug 19, 2019 · 7 comments
Open

String IgnoreCase extension method #30626

BluePositive opened this issue Aug 19, 2019 · 7 comments
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Runtime
Milestone

Comments

@BluePositive
Copy link

How about to split the Equals String extension method in 2 Equals() and EqualsIgnoreCase()
each should a have a default behavior and accepts a new Comparison Enum like the StringComparison Enum that has only 3 values

  1. CurrentCulture
  2. InvariantCulture
  3. Ordinal

as a parameter (like Equals() now)
I think it will make it easier to use for most cases (and more likely to be noticed with an IDE )

@Gnbrkm41
Copy link
Contributor

Gnbrkm41 commented Aug 20, 2019

Duplicate of https://github.com/dotnet/corefx/issues/649?

@Gnbrkm41
Copy link
Contributor

I don't understand what you mean by "you can alter it by passing a StringComparison Enum". Does that mean that your proposed EqualsIgnoreCase should accept StringComparison as the second parameter? at that point might as well as just use the existing Equals method.

@BluePositive
Copy link
Author

@Gnbrkm41 I edited the title to make it more understandable.

@Gnbrkm41
Copy link
Contributor

So, are you suggesting that your suggestion would be used like:

str1.EqualsIgnoreCase(str2, StringComparison.Ordinal)

and it's better than

str1.Equals(str2, StringComparison.OrdinalIgnoreCase)

?

@BluePositive
Copy link
Author

@Gnbrkm41 in most cases it will be

str1.EqualsIgnoreCase(str2)

and that is more straightforward than

str1.Equals(str2, StringComparison.OrdinalIgnoreCase)

@Gnbrkm41
Copy link
Contributor

Gnbrkm41 commented Aug 24, 2019

  1. Why does this has to be an extension method? might as well as add it as an instance method if it's going to be useful for discoverability.

2. I think there's a potential concern with the default culture; every other instance methods (e.g. IndexOf, Compare etc...) use current culture when not specified. This suggestion does not however and use Ordinal, and this would break the tradition of defaulting to culture sensitive comparison. Unless people read the documents (which usually people don't), they would assume that this is culture sensitive, case insensitive comparison. Apparently I was thinking that string.Equals(string) defaults to current culture when its not. 😅

  1. If it's expected to be used without the second parameter for the most case, why bother providing one at all? I think we could just have EqualsIgnoreCase(string) one. It seems rather weird to be able to also specify case insensitivity by StringComparison. (e.g. str1.EqualsIgnorecase(str2, StringComparison.OrdinalIgnoreCase)

p.s: if we're going to add this as an instance method, I think #14065 suits better.

@BluePositive
Copy link
Author

@Gnbrkm41

  1. You're right it does not need to be an extension method.
  2. About your issue about the second parameter, my suggestion is that it shouldn't need the second
    parameter only if you want to change the default.
  3. About your issue with passing a StringComparison.OrdinalIgnoreCase I edit the issue to create a new enum that accepts only 3 values without the IgnoreCase values

@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the Future milestone Feb 1, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@joperezr joperezr added api-needs-work API needs work before it is approved, it is NOT ready for implementation and removed untriaged New issue has not been triaged by the area owner labels Jul 2, 2020
@terrajobst terrajobst removed the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jun 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Runtime
Projects
None yet
Development

No branches or pull requests

6 participants