Review compatibility with Microsoft.Extensions.Configuration

[dodexahedron]

Key is a type that is likely to be in configuration files.

Microsoft.Extensions.Configuration is the standard configuration mechanism, these days.

The following code does not work:

class SomeConfig
{
  public Key SomeKey {get;set;}
}

[Fact]
public void RoundTripForConfiguration_SameAsOriginal
{
  SomeConfig c = new();
  c.SomeKey = Key.G.WithShift;
  File.WriteAllText( "configFile.json", JsonSerializer.Serialize( c ) );
  SomeConfig? roundTrippedConfig = new ConfigurationBuilder( ).AddJsonFile( "configFile.json" ).Build( ).Get<SomeConfig>( );
  Assert.Equal( c.SomeKey, roundTrippedConfig.SomeKey ); // Fails
}

This is because ConfigurationBuilder doesn't respect custom JsonConverters.

One solution is to make a Key ONLY serialize to a simple string value ("Shift+B" in the example code), rather than the whole object, and provide a string cast to Key that parses and returns Key. At least I'm fairly certain that would work, because I've used a similar solution for other projects.

Another is to implement a custom ConfigurationProvider for Microsoft.Extensions.Configuration. However, that is a lot more work and brings in an additional dependency.

The first way is even something that can be achieved without changing the custom serializer that's already there (KeyJsonConverter), since a user can write their own converter to serialize how they want. Such a custom serializer is as simple as this, on the consumer side:

public class AlternativeKeyJsonConverter : JsonConverter<Key>
{
    public override Key Read( ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options )
    {
        if ( Key.TryParse( reader.GetString( ), out Key key ) )
        {
            return key;
        }

        return Key.Empty;
    }

    public override void Write( Utf8JsonWriter writer, Key value, JsonSerializerOptions options )
    {
        writer.WriteStringValue( value.ToString( ) );
    }
}

But that still doesn't fix the incompatibility with configuration, because it's still going to be ignored, even if the user explicitly puts their own [JsonConverter( typeof( AlternativeKeyJsonConverter ) )] attribute on each Key property.

I am not sure if a specialized [JsonConstructor] would help, though I suspect it would not.

Now, while one could say "ok, then they can just JsonSerializer.Deserialize it!" well... no, they can't. Not easily, anyway. That forces the entire configuration to have to be treated that way, or else any configuration containing Keys would have to be kept separate, which is also non-ideal. Plus it would lose the other benefits of the configuration system, such as optional files, automatic configuration reloading, and layered configuration loading.

I think it's fair to say that Microsoft.Extensions.Configuration is not an edge case, so is worth considering, for any type that is likely to be part of a configuration (of any format).

[dodexahedron]

Another option, I suppose, might be to just not include the [JsonConverter (typeof (KeyJsonConverter))] on the Key class. 🤔

[dodexahedron]

Here's another potential option:

The Configuration API looks for a type converter from string, using System.ComponentModel.TypeDescriptor.GetConverter

So, if Key were to also have a TypeConverterAttribute like [TypeConverter(typeof(ATypeThatTurnsStringIntoKey))], perhaps the parser could be coaxed into turning JSON (or other formats) back into keys, so long as an appropriate TypeConverter exists?

Here's skeleton code for a potential TypeConverter for the Key class:

    public class KeyTypeConverter : TypeConverter
    {
        /// <inheritdoc />
        public override bool CanConvertFrom( ITypeDescriptorContext? context, Type sourceType )
        {
            return sourceType switch
            {
                { } when sourceType == typeof( string ) => true,
                { } when sourceType == typeof( Dictionary<string, string> ) => true,
                _ => false
            };
        }

        /// <inheritdoc />
        public override bool CanConvertTo( ITypeDescriptorContext? context, Type? destinationType )
        {
            return destinationType switch
            {
                { } when destinationType == typeof( string ) => true,
                { } when destinationType == typeof( Dictionary<string, string> ) => true,
                _ => false
            };
        }

        /// <inheritdoc />
        public override object? ConvertFrom( ITypeDescriptorContext? context, CultureInfo? culture, object value )
        {
            // Code to attempt turning the types mentioned in CanConvertFrom to a Key object
            return value switch
            {
                string valueAsString => static ( ) =>
                {
                    // Turn a string into a Key, probably by using TryParse or trying Json deserialization if TryParse fails or something
                },
                Dictionary<string, string> valueAsDictionary => static ( ) =>
                {
                    // Turn a key value pair collection into a key, somehow
                },
                _ => null // (or whatever is deemed appropriate)
            };
        }

        /// <inheritdoc />
        public override object? ConvertTo( ITypeDescriptorContext? context, CultureInfo? culture, object? value, Type destinationType )
        {
            if ( value is null )
            {
                return null;
            }

            Key key = (Key)value;
            return destinationType switch
            {
                { } t when t == typeof( string ) => () =>
                {
                    // Something to turn a Key into a string, such as possibly:
                    return JsonSerializer.Serialize( key, new JsonSerializerOptions( ) { Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping } );
                },
                { } t when t == typeof( Dictionary<string, string> ) => static ( ) =>
                {
                    // Code to turn a key into a dictionary
                },
                _ => null
            };
        }
    }

Not only would such a converter likely make the configuration infrastructure happy again, it would be a familiar pattern to those who already are well-versed in TypeConverters from working with XAML-based UIs in .net, since they take advantage of the same mechanism.

[dodexahedron]

Side note: This seems to have overlap with #3023 but is from the perspective of a consumer of the library.

[tig]

Here's skeleton code for a potential TypeConverter for the Key class:

    public class KeyTypeConverter : TypeConverter
    {
        /// <inheritdoc />
        public override bool CanConvertFrom( ITypeDescriptorContext? context, Type sourceType )
        {
            return sourceType switch
            {
                { } when sourceType == typeof( string ) => true,
                { } when sourceType == typeof( Dictionary<string, string> ) => true,
                _ => false
            };
        }

        /// <inheritdoc />
        public override bool CanConvertTo( ITypeDescriptorContext? context, Type? destinationType )
        {
            return destinationType switch
            {
                { } when destinationType == typeof( string ) => true,
                { } when destinationType == typeof( Dictionary<string, string> ) => true,
                _ => false
            };
        }

        /// <inheritdoc />
        public override object? ConvertFrom( ITypeDescriptorContext? context, CultureInfo? culture, object value )
        {
            // Code to attempt turning the types mentioned in CanConvertFrom to a Key object
            return value switch
            {
                string valueAsString => static ( ) =>
                {
                    // Turn a string into a Key, probably by using TryParse or trying Json deserialization if TryParse fails or something
                },
                Dictionary<string, string> valueAsDictionary => static ( ) =>
                {
                    // Turn a key value pair collection into a key, somehow
                },
                _ => null // (or whatever is deemed appropriate)
            };
        }

        /// <inheritdoc />
        public override object? ConvertTo( ITypeDescriptorContext? context, CultureInfo? culture, object? value, Type destinationType )
        {
            if ( value is null )
            {
                return null;
            }

            Key key = (Key)value;
            return destinationType switch
            {
                { } t when t == typeof( string ) => () =>
                {
                    // Something to turn a Key into a string, such as possibly:
                    return JsonSerializer.Serialize( key, new JsonSerializerOptions( ) { Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping } );
                },
                { } t when t == typeof( Dictionary<string, string> ) => static ( ) =>
                {
                    // Code to turn a key into a dictionary
                },
                _ => null
            };
        }
    }

Not only would such a converter likely make the configuration infrastructure happy again, it would be a familiar pattern to those who already are well-versed in TypeConverters from working with XAML-based UIs in .net, since they take advantage of the same mechanism.

What would you think of renaming this issue to "Ensure compatibility with C.E.M"? I suspect there are other Terminal.Gui classes where a type converter like this would be needed.

I think I've addressed the Key related issues you need for Designer, right?

[dodexahedron]

What would you think of renaming this issue to "Ensure compatibility with C.E.M"? I suspect there are other Terminal.Gui classes where a type converter like this would be needed.

Fine with me. As for the type converter, the need for that may have been obviated by having string casts. I'll have to play around with the latest code later.

I think I've addressed the Key related issues you need for Designer, right?

I think so, now, yeah. I'll have a look at that later as well.

[tig]

Worth reading:

https://learn.microsoft.com/en-us/aspnet/core/fundamentals/configuration/options?view=aspnetcore-8.0

[dodexahedron]

Yep. That's the way it's done. It's suuuuuuper-simple.

And it also helps enforce a component of good design by separating configuration types from other types, which has plenty of benefits, not the least of which is making serialization/deserialization of configuration settings significantly simpler. That also isolates configuration from implementation, which is a win for consumers, as a change to the library doesn't necessarily have to also involve a change to configuration.

The ability to have any arbitrary collection of json, xml, yaml, ini, or whatever format configuration files, environment variables, command line arguments, or in-memory objects and just feed them into .Get or .Bind is incredibly powerful and flexible. Heck, I even saw someone who implemented a configuration provider that can grab configuration from an HTTP endpoint. Plus it has the ability to support hot reloading of configuration, for almost free. It also takes care of one of my main gripes with ConfigurationManager, which is that it necessitates a separate specific configuration file. Using the built-in stuff, a consumer can organize the configuration of their application and TG however they like. That's a big win, too.

I you want a real-world example of it in use in an application that has to configure itself, Kestrel, and NLog, check out my SnapsInAZfs project. It keeps configuration in json files by default, but it optionally loads up all files specified by the user, and those are automatically combined by the dotnet configuration infrastructure, and then fed to the components as needed. Users can do anything from cramming everything into one monolithic configuration file all the way down to one setting per file, if they so desired, and it Just Works.

And if a user wanted to use some file format we don't support out of the box, all they have to do is add a reference to a nuget package implementing that format or provide their own, and TG would just use it, without any code change necessary in TG.