From 16d094b68b66dcb47f60c82603c939128a10d86a Mon Sep 17 00:00:00 2001 From: Rauhul Varma Date: Thu, 20 May 2021 12:36:34 -1000 Subject: [PATCH 1/2] Simplify synopsis string generation - Removes unused codepaths. - Simplifies synopsis string codepaths by removing optionality. This complexity is moved to the caller who is now responsible for filtering out hidden arguments and options. This change is desirable as it allows the caller to determine if the argument should be hidden. For example, while it makes sense to hide arguments in help text, it may not make sense to hide them when dumping the arguments for another tool to consume. --- .../BashCompletionsGenerator.swift | 2 +- .../Completions/CompletionsGenerator.swift | 2 +- .../Parsing/ArgumentDefinition.swift | 3 +- .../ArgumentParser/Parsing/ArgumentSet.swift | 9 -- Sources/ArgumentParser/Parsing/Name.swift | 58 ++++++--- .../ArgumentParser/Usage/HelpGenerator.swift | 19 ++- .../ArgumentParser/Usage/UsageGenerator.swift | 118 +++++++----------- 7 files changed, 105 insertions(+), 106 deletions(-) diff --git a/Sources/ArgumentParser/Completions/BashCompletionsGenerator.swift b/Sources/ArgumentParser/Completions/BashCompletionsGenerator.swift index 695f6a8c1..5d03d7036 100644 --- a/Sources/ArgumentParser/Completions/BashCompletionsGenerator.swift +++ b/Sources/ArgumentParser/Completions/BashCompletionsGenerator.swift @@ -145,7 +145,7 @@ struct BashCompletionsGenerator { let commandName = commands.first!._commandName let subcommandNames = commands.dropFirst().map { $0._commandName }.joined(separator: " ") // TODO: Make this work for @Arguments - let argumentName = arg.preferredNameForSynopsis?.synopsisString + let argumentName = arg.names.preferredName?.synopsisString ?? arg.help.keys.first?.rawValue ?? "---" return """ diff --git a/Sources/ArgumentParser/Completions/CompletionsGenerator.swift b/Sources/ArgumentParser/Completions/CompletionsGenerator.swift index 4c45f9f0e..e27c19d27 100644 --- a/Sources/ArgumentParser/Completions/CompletionsGenerator.swift +++ b/Sources/ArgumentParser/Completions/CompletionsGenerator.swift @@ -102,7 +102,7 @@ extension ArgumentDefinition { /// this argument. func customCompletionCall(_ commands: [ParsableCommand.Type]) -> String { let subcommandNames = commands.dropFirst().map { $0._commandName }.joined(separator: " ") - let argumentName = preferredNameForSynopsis?.synopsisString + let argumentName = names.preferredName?.synopsisString ?? self.help.keys.first?.rawValue ?? "---" return "---completion \(subcommandNames) -- \(argumentName)" } diff --git a/Sources/ArgumentParser/Parsing/ArgumentDefinition.swift b/Sources/ArgumentParser/Parsing/ArgumentDefinition.swift index f17c128d0..dba7755b2 100644 --- a/Sources/ArgumentParser/Parsing/ArgumentDefinition.swift +++ b/Sources/ArgumentParser/Parsing/ArgumentDefinition.swift @@ -115,7 +115,7 @@ struct ArgumentDefinition { var valueName: String { help.valueName.mapEmpty { - preferredNameForSynopsis?.valueString + names.preferredName?.valueString ?? help.keys.first?.rawValue.convertedToSnakeCase(separator: "-") ?? "value" } @@ -165,7 +165,6 @@ extension ArgumentDefinition: CustomDebugStringConvertible { extension ArgumentDefinition { var optional: ArgumentDefinition { var result = self - result.help.options.insert(.isOptional) return result } diff --git a/Sources/ArgumentParser/Parsing/ArgumentSet.swift b/Sources/ArgumentParser/Parsing/ArgumentSet.swift index f454da841..21ee983d3 100644 --- a/Sources/ArgumentParser/Parsing/ArgumentSet.swift +++ b/Sources/ArgumentParser/Parsing/ArgumentSet.swift @@ -159,15 +159,6 @@ extension ArgumentSet { extension ArgumentDefinition { /// Create a unary / argument that parses using the given closure. init(key: InputKey, kind: ArgumentDefinition.Kind, parsingStrategy: ParsingStrategy = .default, parser: @escaping (String) -> A?, parseType type: A.Type = A.self, default initial: A?, completion: CompletionKind) { - let initialValueCreator: (InputOrigin, inout ParsedValues) throws -> Void - if let initialValue = initial { - initialValueCreator = { origin, values in - values.set(initialValue, forKey: key, inputOrigin: origin) - } - } else { - initialValueCreator = { _, _ in } - } - self.init(kind: kind, help: ArgumentDefinition.Help(key: key), completion: completion, parsingStrategy: parsingStrategy, update: .unary({ (origin, name, value, values) in guard let v = parser(value) else { throw ParserError.unableToParseValue(origin, name, value, forKey: key) diff --git a/Sources/ArgumentParser/Parsing/Name.swift b/Sources/ArgumentParser/Parsing/Name.swift index 1859c573a..82f4952ca 100644 --- a/Sources/ArgumentParser/Parsing/Name.swift +++ b/Sources/ArgumentParser/Parsing/Name.swift @@ -9,7 +9,7 @@ // //===----------------------------------------------------------------------===// -enum Name: Hashable { +enum Name { /// A name (usually multi-character) prefixed with `--` (2 dashes) or equivalent. case long(String) /// A single character name prefixed with `-` (1 dash) or equivalent. @@ -18,7 +18,9 @@ enum Name: Hashable { case short(Character, allowingJoined: Bool = false) /// A name (usually multi-character) prefixed with `-` (1 dash). case longWithSingleDash(String) - +} + +extension Name { init(_ baseName: Substring) { assert(baseName.first == "-", "Attempted to create name for unprefixed argument") if baseName.hasPrefix("--") { @@ -31,6 +33,35 @@ enum Name: Hashable { } } +// short argument names based on the synopsisString +// this will put the single - options before the -- options +extension Name: Comparable { + static func < (lhs: Name, rhs: Name) -> Bool { + return lhs.synopsisString < rhs.synopsisString + } +} + +extension Name: Hashable { } + +extension Name { + enum Case: Equatable { + case long + case short + case longWithSingleDash + } + + var `case`: Case { + switch self { + case .short: + return .short + case .longWithSingleDash: + return .longWithSingleDash + case .long: + return .long + } + } +} + extension Name { var synopsisString: String { switch self { @@ -53,16 +84,7 @@ extension Name { return n } } - - var isShort: Bool { - switch self { - case .short: - return true - default: - return false - } - } - + var allowsJoined: Bool { switch self { case .short(_, let allowingJoined): @@ -82,10 +104,12 @@ extension Name { } } -// short argument names based on the synopsisString -// this will put the single - options before the -- options -extension Name: Comparable { - static func < (lhs: Name, rhs: Name) -> Bool { - return lhs.synopsisString < rhs.synopsisString +extension BidirectionalCollection where Element == Name { + var preferredName: Name? { + first { $0.case != .short } ?? first + } + + var paritioned: [Name] { + filter { $0.case == .short } + filter { $0.case != .short } } } diff --git a/Sources/ArgumentParser/Usage/HelpGenerator.swift b/Sources/ArgumentParser/Usage/HelpGenerator.swift index 5820cf331..16d997a89 100644 --- a/Sources/ArgumentParser/Usage/HelpGenerator.swift +++ b/Sources/ArgumentParser/Usage/HelpGenerator.swift @@ -161,10 +161,16 @@ internal struct HelpGenerator { let groupEnd = args.firstIndex(where: { $0.help.keys != arg.help.keys }) ?? args.endIndex let groupedArgs = [arg] + args[.. Name? { - let names = getHelpNames() - return names.first(where: { !$0.isShort }) ?? names.first + getHelpNames().preferredName } func versionArgumentDefintion() -> ArgumentDefinition? { diff --git a/Sources/ArgumentParser/Usage/UsageGenerator.swift b/Sources/ArgumentParser/Usage/UsageGenerator.swift index 4d52e60c3..1482aa3dd 100644 --- a/Sources/ArgumentParser/Usage/UsageGenerator.swift +++ b/Sources/ArgumentParser/Usage/UsageGenerator.swift @@ -36,49 +36,48 @@ extension UsageGenerator { /// /// In `roff`. var synopsis: String { - let definitionSynopsis = definition.synopsis - switch definitionSynopsis.count { + // Filter out options that should not be displayed. + var options = definition + .filter { $0.help.shouldDisplay } + switch options.count { case 0: return toolName case let x where x > 12: // When we have too many options, keep required and positional arguments, // but discard the rest. - let synopsis: [String] = definition.compactMap { argument in - guard argument.isPositional || !argument.help.options.contains(.isOptional) else { - return nil - } - return argument.synopsis + options = options.filter { + $0.isPositional || !$0.help.options.contains(.isOptional) } - if !synopsis.isEmpty, synopsis.count <= 12 { - return "\(toolName) [] \(synopsis.joined(separator: " "))" + // If there are between 1 and 12 options left, print them, otherwise print + // a simplified usage string. + if !options.isEmpty, options.count <= 12 { + let synopsis = options + .map { $0.synopsis } + .joined(separator: " ") + return "\(toolName) [] \(synopsis)" } return "\(toolName) " default: - return "\(toolName) \(definition.synopsis.joined(separator: " "))" + let synopsis = options + .map { $0.synopsis } + .joined(separator: " ") + return "\(toolName) \(synopsis)" } } } -extension ArgumentSet { - var synopsis: [String] { - return self - .compactMap { $0.synopsis } - } -} - extension ArgumentDefinition { - var synopsisForHelp: String? { - guard help.shouldDisplay else { return nil } - + var synopsisForHelp: String { switch kind { case .named: - let joinedSynopsisString = partitionedNames + let joinedSynopsisString = names + .paritioned .map { $0.synopsisString } .joined(separator: ", ") - + switch update { case .unary: - return "\(joinedSynopsisString) <\(synopsisValueName ?? "")>" + return "\(joinedSynopsisString) <\(valueName)>" case .nullary: return joinedSynopsisString } @@ -88,15 +87,17 @@ extension ArgumentDefinition { return "" } } - - var unadornedSynopsis: String? { + + var unadornedSynopsis: String { switch kind { case .named: - guard let name = preferredNameForSynopsis else { return nil } - + guard let name = names.preferredName else { + fatalError("preferredName cannot be nil for named arguments") + } + switch update { case .unary: - return "\(name.synopsisString) <\(synopsisValueName ?? "value")>" + return "\(name.synopsisString) <\(valueName)>" case .nullary: return name.synopsisString } @@ -106,34 +107,16 @@ extension ArgumentDefinition { return "" } } - - var synopsis: String? { - guard help.shouldDisplay else { return nil } - - guard !help.options.contains(.isOptional) else { - var n = self - n.help.options.remove(.isOptional) - return n.synopsis.flatMap { "[\($0)]" } + + var synopsis: String { + var synopsis = unadornedSynopsis + if help.options.contains(.isRepeating) { + synopsis += " ..." } - guard !help.options.contains(.isRepeating) else { - var n = self - n.help.options.remove(.isRepeating) - return n.synopsis.flatMap { "\($0) ..." } + if help.options.contains(.isOptional) { + synopsis = "[\(synopsis)]" } - - return unadornedSynopsis - } - - var partitionedNames: [Name] { - return names.filter{ $0.isShort } + names.filter{ !$0.isShort } - } - - var preferredNameForSynopsis: Name? { - names.first { !$0.isShort } ?? names.first - } - - var synopsisValueName: String? { - valueName + return synopsis } } @@ -241,27 +224,20 @@ extension ErrorMessageGenerator { extension ErrorMessageGenerator { func arguments(for key: InputKey) -> [ArgumentDefinition] { - return arguments - .filter { - $0.help.keys.contains(key) - } + arguments + .filter { $0.help.keys.contains(key) } } func help(for key: InputKey) -> ArgumentDefinition.Help? { - return arguments + arguments .first { $0.help.keys.contains(key) } .map { $0.help } } func valueName(for name: Name) -> String? { - for arg in arguments { - guard - arg.names.contains(name), - let v = arg.synopsisValueName - else { continue } - return v - } - return nil + arguments + .first { $0.names.contains(name) } + .map { $0.valueName } } } @@ -313,7 +289,7 @@ extension ErrorMessageGenerator { }) if let suggestion = suggestion { - return "Unknown option '\(name.synopsisString)'. Did you mean '\(suggestion.synopsisString)'?" + return "Unknown option '\(name.synopsisString)'. Did you mean '\(suggestion.synopsisString)'?" } return "Unknown option '\(name.synopsisString)'" } @@ -363,8 +339,10 @@ extension ErrorMessageGenerator { func noValueMessage(key: InputKey) -> String? { let args = arguments(for: key) - let possibilities = args.compactMap { - $0.nonOptional.synopsis + let possibilities: [String] = args.compactMap { + $0.help.shouldDisplay + ? $0.nonOptional.synopsis + : nil } switch possibilities.count { case 0: From a9be190dda7d531917300d7ddd1c5ffc20f2485f Mon Sep 17 00:00:00 2001 From: Rauhul Varma Date: Fri, 21 May 2021 22:35:02 -0500 Subject: [PATCH 2/2] paritioned -> partitioned --- Sources/ArgumentParser/Parsing/Name.swift | 2 +- Sources/ArgumentParser/Usage/UsageGenerator.swift | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Sources/ArgumentParser/Parsing/Name.swift b/Sources/ArgumentParser/Parsing/Name.swift index 82f4952ca..3e7b4b3a0 100644 --- a/Sources/ArgumentParser/Parsing/Name.swift +++ b/Sources/ArgumentParser/Parsing/Name.swift @@ -109,7 +109,7 @@ extension BidirectionalCollection where Element == Name { first { $0.case != .short } ?? first } - var paritioned: [Name] { + var partitioned: [Name] { filter { $0.case == .short } + filter { $0.case != .short } } } diff --git a/Sources/ArgumentParser/Usage/UsageGenerator.swift b/Sources/ArgumentParser/Usage/UsageGenerator.swift index 1482aa3dd..0fc86978e 100644 --- a/Sources/ArgumentParser/Usage/UsageGenerator.swift +++ b/Sources/ArgumentParser/Usage/UsageGenerator.swift @@ -71,7 +71,7 @@ extension ArgumentDefinition { switch kind { case .named: let joinedSynopsisString = names - .paritioned + .partitioned .map { $0.synopsisString } .joined(separator: ", ")