Tyrrrz / CliFx

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

Show default values of non-required options in help #17

Closed Tyrrrz closed 4 years ago

Tyrrrz commented 5 years ago

Suggested approach:

domn1995 commented 4 years ago

To clarify the spec for this feature, how do we want to handle types without "nice" ToString() outputs? Just print out the result of ToString() and if it's ugly it's ugly? For types that implement IEnumerable we can just string.Join() the values of course, though.

Another way to say this is that any of the types supported by the built in value conversions get nicely formatted default values and the rest are not guaranteed. @Tyrrrz, what do you think?

Tyrrrz commented 4 years ago

Totally forgot about that :/

We could check if ToString() is overriden but even if ToString() is overridden on a type, there's no guarantee that it will return the value in the same form it expects to parse. I think for now we should just handle the basic types and IEnumerable<T> of those via string.Join.

domn1995 commented 4 years ago

Yup, that sounds good. How do we want to handle types that do NOT override ToString()?

Ignore them? Print the type name? Print some placeholder value indicating a warning?

Another option is adding a Default property to CommandOptionAttribute where the user can supply a string to print as the default value. If they supply it, we just print that, otherwise we defer to our implementation.

With that last option, we could add an analyzer that checks whether the type doesn't override ToString() and show a warning suggesting they override or set the Default property of the CommandOptionAttribute.

Tyrrrz commented 4 years ago

Let's only handle these types:

https://github.com/Tyrrrz/CliFx/blob/7e4c6b20ffa612214605e1f6f457cd4ff0c0ef70/CliFx/Domain/CommandArgumentSchema.cs#L37-L50

Another option is adding a Default property to CommandOptionAttribute where the user can supply a string to print as the default value. If they supply it, we just print that, otherwise we defer to our implementation.

That would break single source of truth, I don't like that. If the user decides to change the property type or the actual default value, they will have to remember to also update the default string value in attribute.

domn1995 commented 4 years ago

That would break single source of truth, I don't like that. If the user decides to change the property type or the actual default value, they will have to remember to also update the default string value in attribute.

Yeah, I don't like that either, but it would be mostly remedied by an analyzer catching it.

Still not sure what behavior we want for the unsupported types... I think I'll print something like Default: unknown value for now and eventually have an analyzer that warns on it.

Tyrrrz commented 4 years ago

I vote we just don't do anything for unsupported types, i.e. leave the current behavior.

domn1995 commented 4 years ago

I vote we just don't do anything for unsupported types, i.e. leave the current behavior.

I think I'm ok with that if we get an analyzer warning eventually 😎

Tyrrrz commented 4 years ago

@domn1995 after we merge this in, I'm gonna refactor code a bit so don't pick anything else until I'm done :)