[C#] Inconsistent String Comparisons in Schema Field Retrieval Methods

[vthemelis]

In the Schema class, there are three methods used to retrieve individual fields:

public class Schema
{
    // ...

    public Field GetFieldByName(string name) => FieldsLookup[name].FirstOrDefault();

    public int GetFieldIndex(string name, StringComparer comparer)
    {
        IEqualityComparer<string> equalityComparer = (IEqualityComparer<string>)comparer;
        return GetFieldIndex(name, equalityComparer);
    }

    public int GetFieldIndex(string name, IEqualityComparer<string> comparer = default)
    {
        // Implementation using the specified comparer
    }
}

The first method (GetFieldByName) use the default string comparer, while the latter two (GetFieldIndex overloads) are using a culture aware comparator by default (StringComparer.CurrentCulture). This inconsistency can be confusing, as it is unclear when custom comparers are utilized.

Component(s)

C#

[CurtHagenlocher]

I think the best thing to do here might be something like

public static IEqualityComparer<string> DefaultFieldNameComparer = StringComparer.Ordinal;

and then change the instances of StringComparer.CurrentCulture to be DefaultFieldNameComparer. This will give a better default while still letting users revert to the previous behavior globally simply by assigning a different value to the static field.

[vthemelis]

I think the best thing to do here might be something like

public static IEqualityComparer<string> DefaultFieldNameComparer = StringComparer.Ordinal;

and then change the instances of StringComparer.CurrentCulture to be DefaultFieldNameComparer. This will give a better default while still letting users revert to the previous behavior globally simply by assigning a different value to the static field.

Sounds like a good idea but it could invalidate existing lookups or necessitate reindexing lookups on reassignment of its value. What are your thoughts on this?

[adamreeve]

It seems reasonable to expect that users should only set this once on their application startup, before any indexes are created. Maybe a comment warning about the problems that misuse could cause in a public documentation comment would be worthwhile.

[vthemelis]

Hi @adamreeve , happy to change that. I can see some issues with the static field if I'm honest though as this would be impossible to change safely in a library. You wouldn't want including a NuGet package in your executable messing with the behaviour of other libraries that you may be importing.

[CurtHagenlocher]

Yeah, that's a hard problem in general with shared libraries. An alternative is to mark the existing API as deprecated and then create a new one with better behavior. We'd probably want to do this in conjunction with making one or both sets of APIs as lazy so we don't need to pay to e.g. create multiple dictionaries.