spectre.console icon indicating copy to clipboard operation
spectre.console copied to clipboard

Unable to get empty value from nullable instance like TextPrompt<Uri?>

Open danielklecha opened this issue 3 years ago • 5 comments

I want to get AbsoluteUri or null but it's not supported by TextPrompt even with AllowEmpty=true.

var uri = await new TextPrompt<Uri?>( $"abc" )
    .AllowEmpty()
    .Validate( value =>
    {
        if ( !( value?.IsAbsoluteUri ?? true ) )
            return ValidationResult.Error( "[red]It must be absolute url[/]" );
        return ValidationResult.Success();
    } )
    .ShowAsync( AnsiConsole.Console, CancellationToken.None );

If I press enter then I get "Invalid input".

I suppose that input is string.Empty when we call TryConvertFromString. UriConverter allow to convert from string.Empty and return null. In that case condition in line 146 in TextPrompt should be modified and allow result to be null if conversion was successfull.

else if (!TypeConverterHelper.TryConvertFromString<T>(input, out result) || result == null && Nullable.GetUnderlyingType(typeof(T)) == null)
//or maybe
else if (!TypeConverterHelper.TryConvertFromString<T>(input, out result))

This issue could happen for other types if allow to convert from string.Empty and return null. What do you think?


Please upvote :+1: this issue if you are interested in it.

danielklecha avatar Feb 01 '22 13:02 danielklecha

@danielklecha Could you create a minimal reproducable example for this?

patriksvensson avatar Feb 01 '22 14:02 patriksvensson

@patriksvensson Sure 😄 I created console app in .net 6. You must run it and press enter. Then you will see "Invalid input" message. https://github.com/danielklecha/SpectreConsoleIssue

danielklecha avatar Feb 01 '22 18:02 danielklecha

The "problem" is the || result == null in

https://github.com/spectreconsole/spectre.console/blob/0bbf9b81a9a0e5b4a2c4d3e0aab253cf7e13f247/src/Spectre.Console/Widgets/Prompt/TextPrompt.cs#L147

This makes TypeConverters that return null values (like the UriTypeConverter) yield "invalid" results.

I feel this is a bug. E.g. given the following really far-fetched example:

using System.ComponentModel;
using System.Globalization;
using Spectre.Console;

var thing = await new TextPrompt<Thing>("thing")
    .AllowEmpty()
    .ShowAsync(AnsiConsole.Console, CancellationToken.None);
if (thing == null)
    AnsiConsole.WriteLine("You selected a null value");
else
    AnsiConsole.WriteLine($"You selected {thing.Content}");


[TypeConverter(typeof(ThingConverter))]
public class Thing
{
    public Thing(string content)
    {
        Content = content;
    }

    public string Content { get; }
}


public class ThingConverter : TypeConverter
{
    public override bool CanConvertFrom(ITypeDescriptorContext? context, Type sourceType)
    {
        if (sourceType == typeof(string))
        {
            return true;
        }

        return base.CanConvertFrom(context, sourceType);
    }

    public override object? ConvertFrom(ITypeDescriptorContext? context, CultureInfo? culture, object value)
    {
        if (value is string strVal)
        {
            if (string.IsNullOrEmpty(strVal))
            {
                return null;
            }

            if (strVal.EndsWith("a"))
            {
                return null;
            }

            return new Thing(strVal);
        }

        return base.ConvertFrom(context, culture, value);
    }
}

The TypeConverter returns null for all strings ending in a. (As I said: Really far-fetched...) So an (IMHO) valid input of "trallala" would result in an "invalid input" message.

nils-a avatar Feb 09 '22 13:02 nils-a

Could someone modify TextPrompt and update one line? It generates issues with all nullable types.

else if (!TypeConverterHelper.TryConvertFromString<T>(input, out result))

danielklecha avatar Nov 18 '22 19:11 danielklecha

@danielklecha simply removing that || result == 0 will open a long bunch of nullable issues. So, I feel it's not as easy as that. Also, that would be a breaking change, so we'd need to think about that.

We're certainly open to suggestions, though.

/cc @iraklikhitarishvili - this answers your question in #998, probably.

nils-a avatar Nov 22 '22 08:11 nils-a