Allow parsing negative numbers #188
Conversation
There was a problem hiding this comment.
Thanks for this PR, @john-mueller! Unfortunately this approach isn't totally workable, since you can define @Option(name: .customShort("2")) var foo: Int. It doesn't look like we have a test case for this, but we can't treat all negative numerals as numeric values. We need to be able to treat them as values only when they're in a position to be interpreted as such.
For a different approach, you could look at adding something like static var hasDashPrefixedRepresentations: Bool to ExpressibleByArgument, then customizing that for the signed numeric types, and then plumbing it through the parsing to pick up single-dash prefixed values when appropriate. What do you think?
|
Hmm, good point. I wasn't thinking about |
Added case SplitArguments.Element.possibleNegative to delay choosing between a value and an option until more context is known. Fixed bug when an Option taking an array of values used ArrayParsingStrategy.upToNextOption and Name.longWithSingleDash.
john-mueller
force-pushed
the
negative-numbers
branch
from
3af6a10
to
b1bb374
Compare
|
Okay, this should be a fully robust solution. I added a I also found and fixed a bug when using |
There was a problem hiding this comment.
Thanks for your hard work on this, @john-mueller! Your tests look very comprehensive, especially the coverage in NegativeNumbersEndToEndTests.swift. As you’ve seen, there’s quite a bit of subtlety and interaction when parsing option groups, and it looks like you’re exploring that complexity quite well.
I’d still like to constrain this expansion of value matching only to values where we know that a negative number is a valid representation — i.e. floating point and signed integer types. Allowing values like -5 to match for strings or other types doesn’t quite line up with the norms for command-line tools and the different parsing strategies that are already defined in the library. Since you’re parsing (possible) negative values correctly now, the next step would be to look at only actually using these values for specific types that conform to ExpressibleByArgument.
| @@ -142,6 +142,19 @@ extension ArgumentDefinition: CustomDebugStringConvertible { | |||
| } | |||
| } | |||
|
|
|||
| extension ArgumentDefinition: Equatable { | |||
| static func == (lhs: ArgumentDefinition, rhs: ArgumentDefinition) -> Bool { | |||
| lhs.names == rhs.names && lhs.parsingStrategy == rhs.parsingStrategy | |||
There was a problem hiding this comment.
Can you add kind and help to this list as well? We can't test true equality b/c of the closures, but this implementation will return true for any two positional arguments with the same parsing strategy.
| @@ -331,6 +333,28 @@ extension ArgumentSet { | |||
| case .value: | |||
| // We'll parse positional values later. | |||
| break | |||
| case let .possibleNegative(_, parsed): | |||
| // If this matches a short name or long name with single dash, | |||
There was a problem hiding this comment.
This whole section is a bit tricky — can you add examples of what we're dealing with here? e.g. in this first section, with a call of example -5 -5, the first -5 could match a flag and fall through, and then the second -5 would be treated as a potential positional argument. For the second, I guess it's something like example -56, where there are -5 and a -6 flags, neither of which have been used yet?
| func hash(into hasher: inout Hasher) { | ||
| hasher.combine(names) | ||
| hasher.combine(parsingStrategy) | ||
| } |
There was a problem hiding this comment.
Include kind and help here, as well.
| @@ -445,21 +467,22 @@ extension SplitArguments { | |||
| case 0: | |||
| append(.value(arg)) | |||
| case 1: | |||
| let possibleNegativeNumber = Int(arg) != nil || Double(arg) != nil | |||
There was a problem hiding this comment.
| let possibleNegativeNumber = Int(arg) != nil || Double(arg) != nil | |
| let possibleNegativeNumber = Double(arg) != nil |
Double can handle the full range of negative Ints, so just that check should be fine here.
| @@ -432,6 +444,16 @@ extension SplitArguments { | |||
| elements.append((index, element)) | |||
| } | |||
|
|
|||
| /// Append as `.possibleNegative` if it could be a negative value; | |||
| /// otherwise, append as `.option`. | |||
| func appendAsPossibleNegative(if isDashPrefixedNumber: Bool, value: String, option: ParsedArgument) { | |||
There was a problem hiding this comment.
Could we name this appendOption(_:value:isPossibleNegative:)?
|
The Swift project moved the default branch to More detail about the branch update - https://forums.swift.org/t/updating-branch-names/40412 |
Allows negative numbers to be passed as options and arguments.
Fixes #31
Checklist