More consistent API #42
Conversation
djbe
commented
Dec 1, 2016
djbe
commented
- All Option-related types have a validator.
- All Option-related types have a flag parameter.
- All constructors have the same init-parameters order. (Flag constructor arguments order isn't consistent with Option #35)
Sources/ArgumentParser.swift
Outdated
| @@ -239,4 +239,26 @@ public final class ArgumentParser : ArgumentConvertible, CustomStringConvertible | |||
|
|
|||
| return nil | |||
| } | |||
|
|
|||
| /// Returns the value for an option (--name Kyle, --name=Kyle) or flag (-n Kyle) | |||
| public func shiftValueForOption(_ name: String, orFlag flag: Character?) throws -> String? { | |||
There was a problem hiding this comment.
I'm not sure if I like this function name (with orFlag), although I don't have any better ideas at the moment.
Gonna have a think about this.
There was a problem hiding this comment.
Suggestion: make 2 type aliases:
typealias Option = String
typealias Flag = Characterand rename the method to:
shiftValue(for name: Option, or flag: Flag?)| @@ -18,7 +18,7 @@ public func testArgumentDescription() { | |||
| let help = Help([ | |||
| BoxedArgumentDescriptor(value: Option<String>("opt1", "example")), | |||
| BoxedArgumentDescriptor(value: Flag("flag1", description: "an example")), | |||
| BoxedArgumentDescriptor(value: Flag("flag2", default: true)), | |||
There was a problem hiding this comment.
I think I prefer the label being explicit here. It's clearer to the reader what the value true is actually for.
Perhaps it would make more sense to make the default label explicit on all options. The reason it wasn't initially there for Option is because option MUST have a default value.
Sources/Commands.swift
Outdated
| @@ -242,7 +242,7 @@ public func command<A:ArgumentDescriptor, A1:ArgumentDescriptor, A2:ArgumentDesc | |||
| BoxedArgumentDescriptor(value: descriptor5), | |||
| ]) | |||
|
|
|||
| if parser.hasOption("help") { | |||
| if parser.has(option: "help") { | |||
There was a problem hiding this comment.
I'm not certain, but I think this might be discourage from the Swift API guidelines.
https://swift.org/documentation/api-design-guidelines/#argument-labels
There was a problem hiding this comment.
I first removed those, but then the compiler gets confused and I had to add a bunch of typecasts.
You get:
public func has(_ name: Option) -> Bool {}
public func has(_ flag: Flag) -> Bool {}If I then call parser.has("a"), swift will get confused because "a" is a string. This has mostly to do with the unit tests.
Do you mean going back to this?
public func hasOption(_ name: Option) -> Bool {}
public func hasFlag(_ flag: Flag) -> Bool {}
|
I think this PR is almost good to go, just one last thing. We will need to update the examples in the README https://github.com/kylef/Commander#describing-arguments Option("count", 1, description: "The number of times to print.") |
- All Option-related types have a validator. - All Option-related types have a flag parameter. - All constructors have the same init-parameters order.
|
I've rebased this merged this into master. 🍰 |
