Allow suggestions to be replaced

[KathleenDollard]

Issue for PR #955

It took me a minute to get the scenario, but the current WithSuggestions and WithSuggestionSource are actually Add....

[KathleenDollard]

Would it work and be easier to add a ClearSuggestions and ClearSuggestionSources? That would be two lines in the calling code, but it doesn't seem a common case and it would be abundantly clear what is happening (yes, Replace is also clear, but it makes it feel like the current With methods should have been named Add).

It would at least be easier to test :)

[KathleenDollard]

@jmarolf I'm making up your scenario, so can you help with where this is important.

The current suggestions should be the valid values for the type. Scenarios I made up:

• Some values of an enum are not actually allowed (...Suggestions)
• A file or directory info should be relative to something other than the current directory (...Source)
  • Someone may have a solution to this for existing code

[jonsequitur]

The implementation for AddSuggestions is still to add a SuggestionSource so I think a single ClearSuggestionSources should be fine.

[jmarolf]

For me I want to be able to control what the suggestions are. There is api affordance for how to customize the parsing of options but no complete scenario for customizing the suggestion of options. Today if I use an enum and do not stringly-type everything I have no ability control how suggestions work:

1. I am automatically opted into SuggestionSource.ForType(ArgumentType) .GetSuggestions(textToMatch) with no ability to turn it off.
2. I cannot remove or replace suggestions, only add them.

As an aside the: With methods are pretty weird being mutable, fluent methods. I would prefer if these types were actually acting on the types in an immutable way similar to how the with keyword will work in C# 9. To me With means changing some property on an immutable type and returning a new type. Might just be my interaction with other immutable APIs has colored my outlook here so take it with a grain of salt.

[jonsequitur]

Renaming APIs to match people's expectations better is fine. This API actually did start out immutable, hence the name, but we found that it made it harder for people to use.

It's not clear to me that we need these methods that modify and return the same object at all, really. I think it's simpler and less surprising to just do something like this:

var argument = new Argument<MyEnum>(); // default suggestions are based on the type

// to replace all suggestions:
argument.SuggestionSource = new SuggestionSource("one", "two", "three"); // params or IEnerable<T> can be supported 

// or to add suggestions
argument.AddSuggestions( ... );
// or something like:
argument.SuggestionSource = SuggestionSource.Merge( source1, source2 ); ..

Another thing worth mentioning is that this API might get a bit of an update as well to support something a little more Roslyn-like, so we can improve the language services for command lines in the .NET Interactive experience. We want to support things like varying the replacement text and tracking the character positions within an unsplit command line string in order to support hover text and diagnostics.

[jmarolf]

@jonsequitur when I was originally trying out different API sharpes I did just change them to be properties. As you point out, if they are mutable then that's what is going to make the most sense to people. Most mutable APIs in .NET are exposed via properties.

An issue we will have is that AddSuggestion as an extension method will interfere with the instance method of the same name in terms of overload resolution logic.

[jonsequitur]

I think it would be fine to do away with the extension method.

[jmarolf]

what is the breaking change policy here? should I also update the major version number if I am going to propose breaking changes? Should I propose the API shape here first or should I just update the PR?

[jonsequitur]

Since we're still in beta there's no need for a bump to the major version.

We can work out the API shape here or in the PR. I'm fine with either approach.

[jmarolf]

Alright, here's my proposal for what the API changes could look like

src/System.CommandLine/Argument.cs

public class Argument : Symbol, IArgument
{
-	public void AddSuggestions(IReadOnlyCollection<string> suggestions)
-   public void AddSuggestionSource(ISuggestionSource suggest)
-   public void AddSuggestionSource(Suggest suggest)
+   public List<string> Suggestions { get; } = new List<string>();
+   public List<ISuggestionSource> SuggestionSources { get; } = new List<ISuggestionSource>();

+	/// <summary>
+   /// Automatically generate suggestions based on the type
+   /// </summary>
+   public bool AutoGenerateSuggestions { get; set; } = true;

	public override IEnumerable<string?> GetSuggestions(string? textToMatch = null)
	{
	    var fixedSuggestions = Suggestions;
	    var dynamicSuggestions = SuggestionSources
	        .SelectMany(source => source.GetSuggestions(textToMatch));

+	   var typeSuggestions = Enumerable.Empty<string?>();
+	   if (AutoGenerateSuggestions)
+	   {
~	       typeSuggestions = SuggestionSource.ForType(ArgumentType)
	                                         .GetSuggestions(textToMatch);
+	   }

	    return fixedSuggestions
	           .Concat(dynamicSuggestions)
	           .Concat(typeSuggestions)
	           .Distinct()
	           .OrderBy(c => c, StringComparer.OrdinalIgnoreCase)
	           .Containing(textToMatch);
	}

}

src/System.CommandLine/ArgumentExtensions.cs

public static class ArgumentExtensions
{
-	public static TArgument WithSuggestions<TArgument>(this TArgument argument, params string[] suggestions) where TArgument : Argument
-	public static TArgument WithSuggestionSource<TArgument>(this TArgument argument, Suggest suggest) where TArgument : Argument
}

src/System.CommandLine/OptionExtensions.cs

public static class OptionExtensions
{
-	public static TOption WithSuggestions<TOption>(this TOption option, params string[] suggestions) where TOption : Option
-   public static TOption WithSuggestionSource<TOption>(this TOption option, Suggest suggest) where TOption : Option
}

[jonsequitur]

A couple thoughts.

I suspect we don't need AutoGenerateSuggestions since we can clear the Suggestions and SuggestionSources lists.

I think it would be simpler for users if we keep the presence of two lists as an internal implementation detail. Effectively, the Suggestions should be just another suggestion source.

[jmarolf]

I suspect we don't need AutoGenerateSuggestions since we can clear the Suggestions and SuggestionSources lists.

even if you can freely manipulate the lists GetSuggestions always calls SuggestionSource.ForType(ArgumentType).GetSuggestions(textToMatch) meaning these results can never be removed from the suggestion list. We could change the API further and have a AddDefaultSuggestionProvider method that could configure this behavior instead.

[jonsequitur]

I would expect SuggestionSource.ForType(ArgumentType) to simply be the default SuggestionSource, so clearing it should be equivalent to AutoGenerateSuggestions = false, no? The question of course is can we keep the create of the suggestion source lazy.

[jmarolf]

So you are thinking:

private List<ISuggestionSource>? _suggestionSources = null;
public List<ISuggestionSource> SuggestionSources 
{ 
    get
    {
        if (_suggestionSources is null)
        {
            _suggestionSources = new List<ISuggestionSource>()
            {
                SuggestionSource.ForType(ArgumentType)
            };
        }

        return _suggestionSources;
    }
}

Are there threading concerns with these types?