Comments (12)
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.
Related Issues (20)
- In AnimationTree and AnimationPlayer, rapid crossfading in Dominant mode can make inconsistent results.
- OpenBSD build is broken due to compilation errors HOT 2
- Editor Feature Profiles: Selection of CheckBox in Tree does not remember folding for Profile that is not set a Current
- Area 2d in child of instantiated scene behaves differently HOT 2
- get_path() in Resource returns a empty string when it is created on disk. HOT 2
- Linux build error "retrieve: program binary cache file is corrupted. Ignoring and removing." HOT 4
- Can not set back '_subresources' param in import setting to default. HOT 1
- There is still a transparency issue with setting ALPHA=1.0 in the shader script
- Geometry2D.merge_polygons will always return polygons with counter-clock-wise HOT 1
- Compatibility OmniLight turns everything green, only for XRCamera HOT 1
- Godot crashes when script window is made floating and the engine is minimized HOT 3
- Tileset custom data shows as index (not name) in select mode (4.3) HOT 2
- [3.x] `ViewportContainer` unexpectedly turns off physics interpolation HOT 1
- Editor interface doesn't recover after resuming Suspend - Windows 7
- Editor crashes doesn't recover after resuming Suspend - Windows 7 HOT 2
- Built-in scripts of child nodes in editor require editor restart to reload HOT 2
- Animation player not playing animation clips for first time HOT 1
- Godot Export generates build with no main scene selected (no warning or error)
- Remote Debug for web exports is sometimes wrong
- Custom features tags are not respected for custom project settings HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from godot.