Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
danielklecha opened this issue Feb 1, 2022 · 5 comments · May be fixed by #1084
Open

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

danielklecha opened this issue Feb 1, 2022 · 5 comments · May be fixed by #1084
Labels
bug Something isn't working

Comments

@danielklecha
Copy link

danielklecha commented Feb 1, 2022

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 👍 this issue if you are interested in it.

@danielklecha danielklecha added the bug Something isn't working label Feb 1, 2022
@patriksvensson
Copy link
Contributor

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

@danielklecha
Copy link
Author

@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

@nils-a
Copy link
Contributor

nils-a commented Feb 9, 2022

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

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

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.

@danielklecha
Copy link
Author

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

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

@danielklecha danielklecha changed the title Unable to get empty value from TextPrompt<Uri?> Unable to get empty value from nullable instance like TextPrompt<Uri?> Nov 19, 2022
@nils-a
Copy link
Contributor

nils-a commented Nov 22, 2022

@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.

@danielklecha danielklecha linked a pull request Nov 22, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Todo 🕑
Development

Successfully merging a pull request may close this issue.

3 participants