swift-argument-parser icon indicating copy to clipboard operation
swift-argument-parser copied to clipboard

Showing the accepted input type

Open BasThomas opened this issue 6 years ago • 7 comments

Like the blog post mentions:

Because highValue is defined as an Int, only valid inputs are recognized, with no manual parsing or casting necessary on your part:

> random ZZZ
Error: The value 'ZZZ' is invalid for '<high-value>'
Usage: random <high-value>

Would it be possible to give an indication that an Int is expected rather than the invalid String?

BasThomas avatar Feb 28 '20 13:02 BasThomas

Sounds like a great addition! Would be useful to look at whether that would make sense in the extended help, too.

natecook1000 avatar Feb 28 '20 16:02 natecook1000

This is indeed a good idea and I thought it would be a good first PR. However, when trying to solve this issue, I bumped into a few questions which I think should be interesting to discuss the answers here.

In ParserError.unableToParseValue, I thought of adding a 5th associated value, named expected of type String. Then, when throwing this error here and here, by having the generic type A, it can easily be converted into a String.

So the questions I got into were:

  1. Is a 5th associated value in .unableToParseValue too much? If not, is a String the best type to add there?
  2. When A is an Optional<T>, should the conversion to the expected string be Optional<T> or simply T? I mean, in the command line, there is no way to pass an optional :) And if we should "extract" T, what's the best approach to do it? I don't like the idea of using string manipulation.
  3. As in the Math example, Options has a values property which is an array of Int. However, the type of A is simply an Int when trying to parse it. What's the best way to inform the user that values accepts one or more of Int?
  4. When the expected parameter is an enum, like in this example in tests, the error message prints the following: "The value 'png' is invalid for '-f '. Expected: Format." Because the user of CLI tool doesn't necessarily knows the internal implementation, they cannot know what are all the cases the enum. Ideally, the error should print the available cases, but that would be achieved by somehow constraining the value to CaseIterable or some other way I cannot think of.

I hope my doubts and suggestions make sense, and I'm looking forward to your feedback. I'll add soon a link to a branch/commit with the initial changes.

natanrolnik avatar Mar 01 '20 21:03 natanrolnik

Thanks for investigating this, @natanrolnik — great questions!

  1. Is a 5th associated value in .unableToParseValue too much? If not, is a String the best type to there?

I think that’s fine, though we probably want it to be a String?, for reasons covered below. We may want to refactor ParserError in the future to collapse some of the common values (InputOrigin in particular).

  1. When A is an Optional<T>, should the conversion to the expected string be Optional<T> or simply T? I mean, in the command line, there is no way to pass an optional :) And if we should "extract" T, what's the best approach to do it? I don't like the idea of using string manipulation.
  2. As in the Math example, Options has a values property which is an array of Int. However, the type of A is simply an Int when trying to parse it. What's the best way to inform the user that values accepts one or more of Int?
  3. When the expected parameter is an enum, like in this example in tests, the error message prints the following: "The value 'png' is invalid for '-f '. Expected: Format." Because the user of CLI tool doesn't necessarily knows the internal implementation, they cannot know what are all the cases the enum. Ideally, the error should print the available cases, but that would be achieved by somehow constraining the value to CaseIterable or some other way I cannot think of.

We probably want to provide this type information only for known types, rather than try to cover all cases, as you illustrate in (4). In addition to enumerations, these can also be custom ExpressibleByArgument-conforming types or even types that are generated through a transform closure, so there will be a significant chunk of uses where we can’t supply anything useful.

For optional and array values, we just want to talk about the Wrapped/Element type, since that’s what the user provided invalid input for. The usage string can be responsible for showing that the argument or option isn’t required or supports multiple values.

Also, since the audience for these messages isn’t necessarily a Swift programmer, the type information provided can more general than the type name. For example, “Expects an integer” instead of “Expects an Int”.

What do you think?

natecook1000 avatar Mar 02 '20 05:03 natecook1000

@natecook1000 Thanks for your answers 😄 So I tried to implement this approach, by defining the following protocol:

public protocol ExpectedArgumentConvertible: ExpressibleByArgument {
    static var expectedArgumentDescription: String { get }
}

Implementing it was trivial, I added the implementation to all types that already conform to ExpressibleByArgument. The problem is that when trying to access the static variable, the compiler knows that A is ExpressibleByArgument, and not ExpectedArgumentConvertible. To solve this, I see 2 ways:

  • Add the expectedArgumentDescription static var to ExpressibleByArgument, making it String? and returning nil by default;
  • Duplicating this method here making type A to be of type ExpectedArgumentConvertible.

Maybe there is something simpler or better to be done, but I cannot think of at the moment. Please let me know if you have any other directions or ideas.

natanrolnik avatar Mar 03 '20 14:03 natanrolnik

Sorry the delayed response, @natanrolnik! I think your first alternative is the right one:

Add the expectedArgumentDescription static var to ExpressibleByArgument, making it String? and returning nil by default

Then for things like CaseIterable enumerations, we can also provide a implementation in extension ExpressibleByArgument where Self: CaseIterable.

natecook1000 avatar May 02 '20 18:05 natecook1000

Hi @natecook1000, no worries 🙂

That's fine. Now I have another problem: a lot has changed since the last time I looked into it.

I noticed that ParserError.unableToParseValue(...) is now called in more places compared to a few weeks ago:

The only place where I easily found the ExpressibleByArgument type to access the added static variable to the protocol, was in ArgumentDefinition.swift:178.

What would you suggest here? Is the protocol accessible in the other places I mentioned above?

natanrolnik avatar May 03 '20 15:05 natanrolnik

We only have access to the ExpressibleByArgument conformance inside the property wrapper inits with that constraint (i.e. those that don't have a transform parameter). We'll need to capture the information to display in the ArgumentDefinition, like we do for the default value: https://github.com/apple/swift-argument-parser/blob/1081d08b1d716e6a68a9cbc258bb309359fbae1e/Sources/ArgumentParser/Parsing/ArgumentDefinition.swift#L32

natecook1000 avatar May 04 '20 15:05 natecook1000