Runtime: Proposal: Make it easier to use StringComparison & StringComparer with GetHashCode.

[jamesqo]

Proposed API

1. Add API to convert a StringComparison to a StringComparer, to avoid bunch of switch-statements in user code (see 'Motivation' section below).
2. Add string.GetHashCode(StringComparison) API, to make it easier/more discoverable to get the culture-dependent, case-insensitive, etc. hash of a string. Currently, you have to do StringComparer.XXX.GetHashCode(str), which is verbose & hard to find.

```c#
public class StringComparer
{
public static StringComparer FromComparison(StringComparison comparisonType);
}

public sealed class String
{
public int GetHashCode(StringComparison comparisonType);
}

### Motivation

If someone has a variable comparison they don't have to do a bunch of switch-cases (we'll do them instead):

```c#
    public static StringComparer FromComparison(StringComparison comparisonType)
    {
        switch (comparisonType)
        {
            case StringComparison.Ordinal:
                return StringComparer.Ordinal;
            case StringComparison.OrdinalIgnoreCase:
                return StringComparer.OrdinalIgnoreCase;
                ...
        }
    }

For more background: See @stephentoub's suggestion in https://github.com/dotnet/corefx/issues/8034#issuecomment-261519913

cc @justinvp @Elion

[svick]

How is StringComparer.Ordinal.GetHashCode(str) more verbose than str.GetHashCode(StringComparison.Ordinal), when it's 2 characters shorter?

[justinvp]

How is StringComparer.Ordinal.GetHashCode(str) more verbose than str.GetHashCode(StringComparison.Ordinal), when it's 2 characters shorter?

I think it's more about discoverability. Not everyone knows that you can use StringComparer to get a hash code. I've seen a bunch of uses of text.ToLowerInvariant().GetHashCode() when StringComparer.OrdinalIgnoreCase.GetHashCode(text) would have been better.

[JonHanna]

To be honest, I do know that you can use StringComparer that way, and I still forget sometimes and go through a case transform.

[jamesqo]

@svick Another thing to mention is that str.GetHashCode(StringComparison.Ordinal) has less dots, so you have to hit Enter less in VS.

[svick]

@jamesqo I count 2 dots in StringComparer.Ordinal.GetHashCode(str) and 2 dots in str.GetHashCode(StringComparison.Ordinal). 😉

[jamesqo]

@svick Sorry, my logic was faulty yesterday :smile: I was trying to say that, with an instance method, once you type GetHashCode( VS will detect that the method accepts an enum & will offer autocomplete for StringComparison. Whereas, with the current scenario, you need to type at least up to StringC before VS guesses StringComparer, otherwise you need to move the selection down.

Anyway, @justinvp's argument is the more important one. Regardless if one version is 2 characters shorter or has better completion, it needs to be easier for a newbie to discover how to do these things by looking at the methods on String, instead of scouring the whole System namespace.

[jnm2]

Is there any reason that it shouldn't be better to encourage people to be hanging onto IEqualityComparer<string> in the first place rather than StringComparison? Their code is more open for extension that way.

[jamesqo]

@jnm2 I see your point. If we accepted an IEqualityComparer<string>, however, it would not help with discoverability since people would have no way of knowing to plug in a StringComparer.

[jnm2]

@jamesqo <see cref="StringComparer"/>?

[jamesqo]

@jnm2 An XML doc comment does not help much-- some people might not read it. Besides, if you already have a StringComparer you can do _yourComparer.GetHashCode(str) whereas that is not the case with StringComparison (with this proposal, you'd need to write StringComparer.FromComparison(_comparison).GetHashCode(str) which is kinda verbose).

[AlexGhiondea]

@jamesqo I think this makes sense. Marking ready for review.

[karelz]

FYI: Top post reformatted to match "formal API proposal" form - it is easier to read and comprehend in reviews.

[terrajobst]

Looks good as proposed.

[AlexRadch]

@karelz I am working on this issue.

[karelz]

We still need tests

[karelz]

The PR dotnet/corefx#14623 seems to be abandoned. Unassinging the issue - it is "up for grabs" again, available for anyone to pick it up and finish it.

[karelz]

@MikevanDongen I have sent you contributor invite - once you accept, we can assign this issue to you. Please ping me here when you accept.

In future, please ping issues when you grab them, so that others know as well and we can assign them to you. Thank you!

[danmosemsft]

@karelz @MikevanDongen this issue also tracks exposing String.GetHashCode(StringComparison comparisonType); from CoreCLR, which I see was implemented. @MikevanDongen do you plan to put up another PR to expose that one?

/cc @stephentoub re unexposed API.

[MikevanDongen]

I don't mind doing so (later this week).

-------- Original Message --------
From: Dan Moseley [e-mail]
Sent: Sunday, April 30, 2017 11:43 PM
To: dotnet/corefx [e-mail]
Subject: Re: [dotnet/corefx] Proposal: Make it easier to use StringComparison & StringComparer with GetHashCode. (#13800)
CC: Mike van Dongen [e-mail],Mention [e-mail]

@karelz @MikevanDongen this issue also tracks exposing String.GetHashCode(StringComparison comparisonType); from CoreCLR, which I see was implemented. @MikevanDongen do you plan to put up another PR to expose that one?

/cc @stephentoub re unexposed API.

--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
https://github.com/dotnet/corefx/issues/13800#issuecomment-298259227

[danmosemsft]

@MikevanDongen this API is approved to go in so long as the PR is committed before 5/10 when we fork off. After that it would need shiproom ok which might not happen. Just let us know if you don't think you'll be able to get to it soon enough and maybe we can find someone. Thanks :)

@karelz

[karelz]

Let's keep it unassigned, until someone grabs that up. Whoever picks it up, please comment here .... first commenter wins the issue ;-)

[danmosemsft]

Maybe @hughbe would like the honor of creating a method on String...

[jnm2]

What's left to do, add String.GetHashCode to src\System.Runtime\ref\System.Runtime.cs and put tests on it in src\System.Runtime\tests\System\StringTests.netcoreapp.cs?

[jnm2]

I can't find StringComparer in src\System.Runtime\ref\System.Runtime.cs. Where is that exposed?

Found it.

[jnm2]

Almost done writing tests, so I guess I'll take this. :-p

[jnm2]

https://github.com/dotnet/corefx/pull/19443

[karelz]

Thanks @jnm2! Assigned to you ...

[hughbe]

Thanks for taking this on! Although dan suggested I give it a go I wouldn't be able to find time before ZBB so it's awesome to see