Fix generated shell completion script handling of repeating & non-repeating positional arguments, flags & options #808
Conversation
rgoldberg
changed the title
Signed-off-by: Ross Goldberg <[email protected]>
…wift-format violation. swift-format seems to have a bug. Signed-off-by: Ross Goldberg <[email protected]>
Signed-off-by: Ross Goldberg <[email protected]>
Signed-off-by: Ross Goldberg <[email protected]>
Signed-off-by: Ross Goldberg <[email protected]>
Signed-off-by: Ross Goldberg <[email protected]>
Signed-off-by: Ross Goldberg <[email protected]>
rgoldberg
force-pushed
the
806-repeating-arguments
branch
from
c819a12 to
1ae39ef
Compare
| \(declareTopLevelArray)repeating_options=(\(options.filter(\.isRepeating).flatMap(\.completionWords).joined(separator: " "))) | ||
| \(declareTopLevelArray)non_repeating_options=(\(options.filter { !$0.isRepeating }.flatMap(\.completionWords).joined(separator: " "))) | ||
| \(offerFlagsOptionsFunctionName) \ | ||
| \(positionalArguments.contains { $0.isRepeating } ? 9_223_372_036_854_775_807 : positionalArguments.count) |
There was a problem hiding this comment.
This Int.max literal doesn't seem platform-safe – can you say a bit about the role it's serving?
There was a problem hiding this comment.
If a positional arg is defined after a repeating positional arg, the prior repeating positional arg swallows all the positional arguments, so I wanted to ensure that no completion script is generated for any positional args after the first repeating positional arg.
I used the magic number to accept all non-flag/non-option/non-option-value arguments as positionals if there is any repeating positional.
I forgot that Swift runs on non-Macs…
Thanks for catching this.
There was a problem hiding this comment.
@natecook1000 Just pushed a fix for this. I also rewrote my earlier reply because it was wrong, as I misremembered exactly what the magic number was for; the solution was the same either way, but I realized I misspoke about its exact purpose when I went in to fix it. Early Monday isn't my best time for remembering stuff I haven't looked at for over a week… :)
| // If there aren't any, skip the case block altogether. | ||
| let optionHandlers = | ||
| (arguments ?? []).compactMap { arg in | ||
| arguments.compactMap { arg in |
There was a problem hiding this comment.
Should we use options from above here?
There was a problem hiding this comment.
Yes, we should. Thanks for catching that. This prompted me to notice another older overlooked failure to use an earlier variable. Will fix both.
There was a problem hiding this comment.
Just submitted a fix for this. Will fix the magic number issue now.
…eneration. Signed-off-by: Ross Goldberg <[email protected]>
… accept all subsequent non-flag/non-option/non-option-value tokens as positionals. Signed-off-by: Ross Goldberg <[email protected]>
rgoldberg
force-pushed
the
806-repeating-arguments
branch
from
ed5bcf3 to
eb86da3
Compare
|
@natecook1000 Thanks for the feedback. I've submitted fixes for the 2 issues you raised. Thanks again. |
Fix generated shell completion script handling of repeating & non-repeating positional arguments, flags & options.
Also fix an unrelated swift-format violation & DocC typos in
Flag.swift.Resolve #806
Checklist