Tyrrrz / CliFx

Class-first framework for building command-line interfaces
MIT License
1.48k stars 60 forks source link

Allow scalar option/parameter types be constructed with arrays of strings #57

Closed SingleAccretion closed 4 years ago

SingleAccretion commented 4 years ago

Proposal

Allow the supported types of options/parameters be constructed with enumerables of strings, not just raw strings. This is already essentially supported, but with a "wart" in a form of a requirement to implement a dummy interface (see below). For this reason, the proposal concerns only adding the support for new(ICollectionType<string> strings) signature.

Motivation

To provide a better user experience, I have been using arrays of strings instead of raw strings to not have to use quotes when specifying argument values:

[CommandParameter(0)]
string[] BookName { get; set; }
//add book The Book With A Long Title 
//instead of
//add book "The Book With A Long Title"

I decided to take advantage of CliFX's default handling of invalid/missing arguments, so I created a strong type for BookName to replace string[]:

public class Name
{
   public Name(string[] strings) => Value = string.Join(' ', strings);
   public string Value { get; set; }
}

This, unfortunately, does not work in the current version (in the README file, there is a mention of types with constructors of supported types also being supported, which lead to me to believe this was possible in the first place, but this could be attributed to me misinterpreting the documentation, since this point is under "Supports collection types" header, which logically excludes scalars). What does work (as expected), however, is the following workaround:

public class Name : IEnumerable
{
   public Name(string[] strings) => Value = string.Join(' ', strings);
   public string Value { get; set; }
   IEnumerator IEnumerable.GetEnumerator() => throw new NotImplementedException();
}

Thus I decided to file this issue to see if there would be a possibility of adding "full" support for this pattern.

Implementation

As far as I understand, this would involve refactoring the logic here to support the notion of an enumerable type being not just a type with enumerableUnderlyingType == null, but also one with the appropriate constructor. A bigger refactoring could be considered, and the notion of "fake" enumerable types could be introduced, but the determination of need for that requires a deeper understanding of the codebase, which I do not (yet) have. It would seem reasonable to me to assume that the fact that the value is a "real" enumerable is of questionable relevance, since that information could only be useful if there is a need to actually call the iterator methods, which the library currently does not do, at least according to the GitHub search for GetEnumerator.

Tyrrrz commented 4 years ago

Hi! Thanks for such a detail post. I see that the main motivation behind all of this is that you wanted to avoid quotes when specifying values with spaces. But is it really worth it? I have personally never found it bad and I would assume most users are already familiar with the notion of using quotes to escape special characters as it's used everywhere throughout. Additionally it would mess with the help text because it would say add book <values...> instead of add book <value>. The interface IEnumerable or IEnumerable<T> is actually used. It's reflected upon to determine the type of the elements in the sequence (object for non-generic IEnumerable and T for generic) and then convert the string values to that type. Since you have a non-generic IEnumerable, the strings are just converted to object[] which is pretty straightforward. It seems to me that it's a bit too much of an extreme scenario to add support for this in the library itself. As you've found, you can work around it by implementing a dummy IEnumerable. Alternatively you can just opt to not use a custom object and just join strings the inside your ICommand.ExecuteAsync().

SingleAccretion commented 4 years ago

I see that the main motivation behind all of this is that you wanted to avoid quotes when specifying values with spaces. But is it really worth it?

Well, my use case was a little CRUD application with console as a UI, and avoiding quotes when typing, e. g. book names, is quite nice, but I can very much see this being a niche scenario. Perhaps my stronger argument was that this was "already supported", and so implementing it would be "easy", but that turns out to not be the case, so this argument is invalid. I guess I'll close the issue then.