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

Proposal: Add allowedValues to Option and Argument, surface in --help

Open bdrelling opened this issue 4 years ago • 9 comments

Problem Statement

Given the following command:

struct List: ParsableCommand {
    private enum Filter: String, ExpressibleByArgument {
        case all
        case devices
        case deviceTypes
        case runtimes
        case pairs
    }
    
    @Option(default: .all, help: "Allows filtering of list output.")
    private var list: Filter
}

When I call swift run list --help, I only see the following:

OVERVIEW: Lists all simulator devices, device types, and runtimes available.

USAGE: list [--mode <mode>]

OPTIONS:
  --filter <filter>       Allows filtering of specific list output. (default: all)

Proposed Solution

We could add the property allowedValues: [Value]? to the Option and Argument property wrappers, simply referenced as allowed in the initializer. Based on the previous example, it could be declarable like so:

@Option(default: .all, allowed: [.all, .devices, .deviceTypes], help: "Allows filtering of list output.")

If @Option happens to be applied to value that implements CaseIterable, we could default to .allCases for the value as well if it is left nil. That would further simplify the declaration.

The output of swift run list --help would now look something like:

  --filter <filter>       Allows filtering of specific list output. (default: all) (allowed: devices, deviceTypes, pairs, runtimes)

Notice also that it excludes the default value, so as to reduce unnecessary detail. I'm not sure if doing so would have any negative effects. I'm not really sold on adding it to the end versus surfacing it some other way that commands might (simple array syntax, showing it under the option, etc.).

Additional Thoughts

I'm torn on whether or not we should specifically validate against this list--my gut says that if someone wants that functionality, they could simple use an enum-backed Option. I would also find it beneficial to continue to have "secret" options available--the allowedValues are the ones shown to the user, but the actual validated values are still triggered when the command is sent and handled.

bdrelling avatar May 15 '20 04:05 bdrelling

Thanks for this writeup, @bdrelling! I definitely agree that we want to include allowed values in the help text, but I think the right place to handle their definition is at the type level instead of at the individual property level. The ExpressibleByArgument protocol seems like the right place for that, since we could offer an empty default implementation, with a customized version for CaseIterable types. What would you think of something like this?

protocol ExpressibleByArgument {
    // existing requirements...
    
    static var allowedValues: [String] { get }
}

extension ExpressibleByArgument {
    static var allowedValues: [String] { return [] }
}

extension ExpressibleByArgument where Self: CaseIterable {
    static var allowedValues: [String] {
        allCases.map { String(describing: $0) }
    }
}

Then if you add CaseIterable conformance to your Filter type, you get everything you need without additional configuration at the option/argument declaration site.

natecook1000 avatar May 16 '20 17:05 natecook1000

Oh, I like that a lot for enums! That’s where my head was at initially, but I was thinking that a non-enum property might still want a list of allowedValued, or an enum property might want to not include all cases. What are your thoughts with that?

bdrelling avatar May 16 '20 17:05 bdrelling

Those both feel more like edge cases than would justify increasing the API surface at the declaration site. For the first case, you can add any extra information about what's allowed to the abstract or discussion. For the second, you could define a separate enum to represent the user-available values and map from one to the other. E.g. you'd define your property in terms of NoDeviceFilter here:

enum Filter: String {
    case all, devices, deviceTypes, runtimes, pairs
}

enum NoDeviceFilter: String, CaseIterable, ExpressibleByArgument {
    case all, runtimes, pairs
    
    var asFilter: Filter {
        switch self {
        case .all: return .all
        case .runtimes: return .runtimes
        case .pairs: return .pairs
        }
    }
}

natecook1000 avatar May 17 '20 19:05 natecook1000

Note that this ends up overlapping quite a bit with #8 if we go the ExpressibleByArgument route.

natecook1000 avatar May 17 '20 19:05 natecook1000

I started work on something similar (for CaseIterable), but I'm worried I'm not finding all the possible call points.

Wevah avatar Aug 02 '20 18:08 Wevah

A related issue is that the options themselves may have multiple names/spellings or long/short version, for example

enum FormatType: ExpressibleByArgument,CaseIterable {
    case double
    case int
    init?(argument: String) {
        switch argument {
        case "double", "d": self = .double
        case "int","i": self = .int
        default: return nil
        }
    }
}

with a reasonable help such as

OPTIONS:
  --format-type <format>  The type to use (default: double)
        d,double: range is in doubles
        i,int: range is an int

One approach would be to allow each instance of ExpressibleByArgument to contribute a help type, which is then assembled by the ExpressibleByArgument where Self: CaseIterable extension to collect all the cases.

drewcrawford avatar Oct 05 '20 21:10 drewcrawford

For now a work around:

@Option(help: "Set the logging level. This is helpful in debugging connection, authentication, and configuration problems. (allowed: \(Logger.Level.allValueStrings.joined(separator: ", ")))")
var logLevel: Logger.Level = .error

Ends up looking like:

--log-level <log-level> Set the logging level. This is helpful in debugging connection, authentication, and configuration problems. (allowed: trace, debug, info, notice,
                        warning, error, critical) (default: error)

Maybe not perfect but serviceable. Sorry for the noise, might be obvious to others 🤷

RLovelett avatar Apr 09 '21 21:04 RLovelett

Excellent work around @RLovelett , will be using that for now until a feature like that is added.

ptrkstr avatar Sep 10 '21 05:09 ptrkstr

Now that ExpressibleByArgument has allValueStrings, I think we don't need to add any more properties, I think the help printer just needs to print it?

ian-twilightcoder avatar Sep 01 '22 05:09 ian-twilightcoder