StringExtensions.CompareTo leads to IndexOutOfRangeException on non-empty strings about godot HOT 12 CLOSED

To be honest, I don't know what we're not simply calling CompareInfo.Compare(string, string, CompareOptions) here.

from godot.

This wouldn't be the first time a StringExtensions method was deprecated in favor of a native alternative; I say we just go for that.

from godot.

I ran a quick test to check if String.Compare would work and found out that:

Mathf.Clamp(String.Compare(instance, to, caseSensitive? StringComparison.Ordinal : StringComparison.OrdinalIgnoreCase), -1, 1)

outputs a different result to your originally suggested fix (index >= stringVariable.Length) ~50% of the time.

I don't know if this will cause any issues. Maybe it's risky to have inconsistent results between c# and GDScript?
At the same time it might be more risky for the StringExtensions method to not match the expected C# behaviour, in which case we should also drop the [-1,1] range...

from godot.

TBH, even deprecated, the method probably should not NRE. It doesn't change the fact that that needs to be addressed.

from godot.

My thought was similar. It looks like it fell under the Radar because C# provides these functions already and calling string.CompareTo(string) does not call the StringExtensions method of Godot but the standard .NET method in which everything works fine. I only actually noticed it because I added the boolean argument.

For someone who wants to exercise using Git pull requests, it could look like this:

public static int CompareTo(this string instance, string to, bool caseSensitive = true)
{
    return String.Compare(instance, to, caseSensitive? StringComparison.Ordinal : StringComparison.OrdinalIgnoreCase);
}

from godot.

Using invariant instead of ordinal should match the implementation more closely IMHO.

from godot.

You are right, I re-ran the test using Invariant and got a 100% match for both String.Compare and InvariantCulture.CompareInfo.Compare.

from godot.

Ah, I didn't know, GDScript uses an Invariant Culture like comparison. The original C# implementation (that I linked above) looks perfectly ordinal to me (instance[instanceIndex] < to[toIndex]) and using InvariantCulture comparison could be a breaking change, or I am missing something. But people get Invariant Culture comparison when they don't provide the boolean 2nd argument because Invariant Culture comparison is the comparison used by C#'s own string.CompareTo(). Maybe this means, InvariantCulture comparison indeed reduces the surprises (between passing a 2nd argument true and no second argument). The behaviour just needs to be documented properly.

from godot.

Ah, I didn't know, GDScript uses an Invariant Culture like comparison.

It does not, it is ordinal. But I'd still be inclined to match default BCL behaviour here, as I think that's what most people would expect.

About the change being breaking, yes, but the current behaviour we are breaking is an NRE so I'm not sure how much of a consideration that really is.

Cc @raulsntos @Repiteo since we talked quite a lot about cultures and comparisons a while ago.

from godot.

While I'm generally in favor of matching BCL behavior over GDScript behavior, the purpose of StringExtensions is to match the Godot string methods. In this particular case, it may be better for C# users to just use string.CompareTo with a StringComparison parameter directly which allows you to get the exact behavior you want.

Personally, I'd just deprecate this method since we already have string.CompareTo in the BCL. But last time I proposed this I was told that the boolean parameter was more convenient over the StringComparison parameter so it was worth keeping it.

Regarding changing behavior, @paulloz is right. Since the current behavior is a bug (resulting in an exception), it's fine to change behavior here.

from godot.

It's not technically a behaviour bug (the behaviour is ordinal comparison, the bug is the nul-termination assumption) but never mind, just make sure to document the choice, e.g. here.

I would fully support deprecating this Godot method because there is a BCL-type method String.Compare(String, String, Boolean) https://learn.microsoft.com/en-us/dotnet/api/system.string.compare?view=net-8.0#system-string-compare(system-string-system-string-system-boolean) which already does string comparison with boolean ignoreCase parameter. Typing just an additional String class name in front is not a valid argument about less convenience as I think.

from godot.

I didn't realize there was also an overload with a boolean, I must have confused this with another method. In that case I agree that we should just deprecate it.

from godot.