From 6f503cfdc872aec2ff091115005ed048f12c63b1 Mon Sep 17 00:00:00 2001 From: ibrahimoktay Date: Thu, 4 Jun 2020 12:35:14 +0300 Subject: [PATCH 1/5] Use the first long name in usage (#167) Sorting names for option has been changed. * Usage will choose the first long name, if available, or otherwise the first short name * Help screen will show short names, then long names, in the order of their declaration. Solves (#167) --- .../NameSpecification.swift | 4 ++-- .../ArgumentParser/Usage/UsageGenerator.swift | 22 ++----------------- .../HelpGenerationTests.swift | 19 ++++++++++++++++ .../UsageGenerationTests.swift | 12 ++++++++++ 4 files changed, 35 insertions(+), 22 deletions(-) diff --git a/Sources/ArgumentParser/Parsable Properties/NameSpecification.swift b/Sources/ArgumentParser/Parsable Properties/NameSpecification.swift index a719ee637..18132cb4d 100644 --- a/Sources/ArgumentParser/Parsable Properties/NameSpecification.swift +++ b/Sources/ArgumentParser/Parsable Properties/NameSpecification.swift @@ -38,10 +38,10 @@ public struct NameSpecification: ExpressibleByArrayLiteral { /// Short labels can be combined into groups. case customShort(Character) } - var elements: Set + var elements: Array public init(_ sequence: S) where S : Sequence, Element == S.Element { - self.elements = Set(sequence) + self.elements = Array(sequence) } public init(arrayLiteral elements: Element...) { diff --git a/Sources/ArgumentParser/Usage/UsageGenerator.swift b/Sources/ArgumentParser/Usage/UsageGenerator.swift index 4a83c699b..f7e2d1f48 100644 --- a/Sources/ArgumentParser/Usage/UsageGenerator.swift +++ b/Sources/ArgumentParser/Usage/UsageGenerator.swift @@ -114,29 +114,11 @@ extension ArgumentDefinition { } var sortedNames: [Name] { - return names - .sorted { (lhs, rhs) -> Bool in - switch (lhs, rhs) { - case let (.long(l), .long(r)): - return l < r - case (_, .long): - return true - case (.long, _): - return false - case let (.short(l), .short(r)): - return l < r - case (_, .short): - return true - case (.short, _): - return false - case let (.longWithSingleDash(l), .longWithSingleDash(r)): - return l < r - } - } + return names.filter{ $0 != .long($0.valueString) } + names.filter{ $0 == .long($0.valueString) } } var preferredNameForSynopsis: Name? { - sortedNames.last + names.first{ $0 == .long($0.valueString) } ?? names.first } var synopsisValueName: String? { diff --git a/Tests/ArgumentParserUnitTests/HelpGenerationTests.swift b/Tests/ArgumentParserUnitTests/HelpGenerationTests.swift index 822bf6126..1e9d723fe 100644 --- a/Tests/ArgumentParserUnitTests/HelpGenerationTests.swift +++ b/Tests/ArgumentParserUnitTests/HelpGenerationTests.swift @@ -358,4 +358,23 @@ extension HelpGenerationTests { """) } + + struct L: ParsableArguments { + @Option( + name: [.short, .customLong("remote"), .customLong("when"), .long, .customLong("there"), .customShort("x"), .customShort("y")], + help: "Help Message") + var time: String? + } + + func testHelpWithMultipleCustomNames() { + AssertHelp(for: L.self, equals: """ + USAGE: l [--remote ] + + OPTIONS: + -t, -x, -y, --remote, --when, --time, --there + Help Message + -h, --help Show help information. + + """) + } } diff --git a/Tests/ArgumentParserUnitTests/UsageGenerationTests.swift b/Tests/ArgumentParserUnitTests/UsageGenerationTests.swift index caeea437b..f1a2be4d0 100644 --- a/Tests/ArgumentParserUnitTests/UsageGenerationTests.swift +++ b/Tests/ArgumentParserUnitTests/UsageGenerationTests.swift @@ -149,4 +149,16 @@ extension UsageGenerationTests { let help = UsageGenerator(toolName: "bar", parsable: J()) XCTAssertEqual(help.synopsis, "bar --req [--opt ]") } + + struct K: ParsableArguments { + @Option( + name: [.short, .customLong("remote"), .customLong("when"), .customLong("there")], + help: "Help Message") + var time: String? + } + + func testSynopsisWithMultipleCustomNames() { + let help = UsageGenerator(toolName: "bar", parsable: K()) + XCTAssertEqual(help.synopsis, "bar [--remote ]") + } } From a3ef36baf5b019b7d3e9ea4defdc50b398b44114 Mon Sep 17 00:00:00 2001 From: ibrahimoktay Date: Fri, 5 Jun 2020 08:39:32 +0300 Subject: [PATCH 2/5] Fix adding same name more than once. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Checkin long names in sort caused single dash long names to be treated as a short name. Fixed sort to check “short” instead of “long” Update current test to cover more cases. --- .../Parsable Properties/NameSpecification.swift | 8 +++++++- Sources/ArgumentParser/Usage/UsageGenerator.swift | 4 ++-- .../HelpGenerationTests.swift | 4 ++-- .../UsageGenerationTests.swift | 12 ++++++++++++ 4 files changed, 23 insertions(+), 5 deletions(-) diff --git a/Sources/ArgumentParser/Parsable Properties/NameSpecification.swift b/Sources/ArgumentParser/Parsable Properties/NameSpecification.swift index 18132cb4d..121230754 100644 --- a/Sources/ArgumentParser/Parsable Properties/NameSpecification.swift +++ b/Sources/ArgumentParser/Parsable Properties/NameSpecification.swift @@ -41,7 +41,13 @@ public struct NameSpecification: ExpressibleByArrayLiteral { var elements: Array public init(_ sequence: S) where S : Sequence, Element == S.Element { - self.elements = Array(sequence) + let array = Array(sequence) + self.elements = Array() + for element in array { + if !self.elements.contains(element) { + self.elements.append(element) + } + } } public init(arrayLiteral elements: Element...) { diff --git a/Sources/ArgumentParser/Usage/UsageGenerator.swift b/Sources/ArgumentParser/Usage/UsageGenerator.swift index f7e2d1f48..5faba2c3b 100644 --- a/Sources/ArgumentParser/Usage/UsageGenerator.swift +++ b/Sources/ArgumentParser/Usage/UsageGenerator.swift @@ -114,11 +114,11 @@ extension ArgumentDefinition { } var sortedNames: [Name] { - return names.filter{ $0 != .long($0.valueString) } + names.filter{ $0 == .long($0.valueString) } + return names.filter{ $0 == .short($0.valueString.first!) } + names.filter{ $0 != .short($0.valueString.first!) } } var preferredNameForSynopsis: Name? { - names.first{ $0 == .long($0.valueString) } ?? names.first + names.first{ $0 != .short($0.valueString.first!) } ?? names.first } var synopsisValueName: String? { diff --git a/Tests/ArgumentParserUnitTests/HelpGenerationTests.swift b/Tests/ArgumentParserUnitTests/HelpGenerationTests.swift index 1e9d723fe..338ad8baa 100644 --- a/Tests/ArgumentParserUnitTests/HelpGenerationTests.swift +++ b/Tests/ArgumentParserUnitTests/HelpGenerationTests.swift @@ -361,7 +361,7 @@ extension HelpGenerationTests { struct L: ParsableArguments { @Option( - name: [.short, .customLong("remote"), .customLong("when"), .long, .customLong("there"), .customShort("x"), .customShort("y")], + name: [.short, .customLong("remote"), .customLong("remote"), .short, .customLong("when"), .long, .customLong("other", withSingleDash: true), .customLong("there"), .customShort("x"), .customShort("y")], help: "Help Message") var time: String? } @@ -371,7 +371,7 @@ extension HelpGenerationTests { USAGE: l [--remote ] OPTIONS: - -t, -x, -y, --remote, --when, --time, --there + -t, -x, -y, --remote, --when, --time, -other, --there Help Message -h, --help Show help information. diff --git a/Tests/ArgumentParserUnitTests/UsageGenerationTests.swift b/Tests/ArgumentParserUnitTests/UsageGenerationTests.swift index f1a2be4d0..43ebc2f80 100644 --- a/Tests/ArgumentParserUnitTests/UsageGenerationTests.swift +++ b/Tests/ArgumentParserUnitTests/UsageGenerationTests.swift @@ -161,4 +161,16 @@ extension UsageGenerationTests { let help = UsageGenerator(toolName: "bar", parsable: K()) XCTAssertEqual(help.synopsis, "bar [--remote ]") } + + struct L: ParsableArguments { + @Option( + name: [.short, .short, .customLong("remote", withSingleDash: true), .short, .customLong("remote", withSingleDash: true)], + help: "Help Message") + var time: String? + } + + func testSynopsisWithSingleDashLongNameFirst() { + let help = UsageGenerator(toolName: "bar", parsable: L()) + XCTAssertEqual(help.synopsis, "bar [-remote ]") + } } From 2ab0c692358606a680947168f20ec4bc10a6e888 Mon Sep 17 00:00:00 2001 From: ibrahimoktay Date: Tue, 9 Jun 2020 20:58:50 +0300 Subject: [PATCH 3/5] Add isShort control to Name *Replace sortedNames with partitionedNames *Extract uniquifying logic into SequenceExtensions.swift --- .../NameSpecification.swift | 8 +------ Sources/ArgumentParser/Parsing/Name.swift | 4 ++++ .../ArgumentParser/Usage/UsageGenerator.swift | 6 ++--- .../Utilities/SequenceExtensions.swift | 22 +++++++++++++++++++ 4 files changed, 30 insertions(+), 10 deletions(-) create mode 100644 Sources/ArgumentParser/Utilities/SequenceExtensions.swift diff --git a/Sources/ArgumentParser/Parsable Properties/NameSpecification.swift b/Sources/ArgumentParser/Parsable Properties/NameSpecification.swift index 121230754..6e5bacba6 100644 --- a/Sources/ArgumentParser/Parsable Properties/NameSpecification.swift +++ b/Sources/ArgumentParser/Parsable Properties/NameSpecification.swift @@ -41,13 +41,7 @@ public struct NameSpecification: ExpressibleByArrayLiteral { var elements: Array public init(_ sequence: S) where S : Sequence, Element == S.Element { - let array = Array(sequence) - self.elements = Array() - for element in array { - if !self.elements.contains(element) { - self.elements.append(element) - } - } + self.elements = sequence.uniquified() } public init(arrayLiteral elements: Element...) { diff --git a/Sources/ArgumentParser/Parsing/Name.swift b/Sources/ArgumentParser/Parsing/Name.swift index c3da58261..bb2c1b095 100644 --- a/Sources/ArgumentParser/Parsing/Name.swift +++ b/Sources/ArgumentParser/Parsing/Name.swift @@ -53,6 +53,10 @@ extension Name { return n } } + + var isShort: Bool { + return self == .short(self.valueString.first!) + } } // short argument names based on the synopsisString diff --git a/Sources/ArgumentParser/Usage/UsageGenerator.swift b/Sources/ArgumentParser/Usage/UsageGenerator.swift index 5faba2c3b..fdd3b5a83 100644 --- a/Sources/ArgumentParser/Usage/UsageGenerator.swift +++ b/Sources/ArgumentParser/Usage/UsageGenerator.swift @@ -63,7 +63,7 @@ extension ArgumentDefinition { switch kind { case .named: - let joinedSynopsisString = sortedNames + let joinedSynopsisString = partitionedNames .map { $0.synopsisString } .joined(separator: ", ") @@ -113,8 +113,8 @@ extension ArgumentDefinition { return unadornedSynopsis } - var sortedNames: [Name] { - return names.filter{ $0 == .short($0.valueString.first!) } + names.filter{ $0 != .short($0.valueString.first!) } + var partitionedNames: [Name] { + return names.filter{ $0.isShort } + names.filter{ !$0.isShort } } var preferredNameForSynopsis: Name? { diff --git a/Sources/ArgumentParser/Utilities/SequenceExtensions.swift b/Sources/ArgumentParser/Utilities/SequenceExtensions.swift new file mode 100644 index 000000000..c1bf74a39 --- /dev/null +++ b/Sources/ArgumentParser/Utilities/SequenceExtensions.swift @@ -0,0 +1,22 @@ +//===----------------------------------------------------------*- swift -*-===// +// +// This source file is part of the Swift Argument Parser open source project +// +// Copyright (c) 2020 Apple Inc. and the Swift project authors +// Licensed under Apache License v2.0 with Runtime Library Exception +// +// See https://swift.org/LICENSE.txt for license information +// +//===----------------------------------------------------------------------===// + +extension Sequence where Element: Equatable { + func uniquified() -> [Element] { + var sequence = Array() + for element in self { + if !sequence.contains(element) { + sequence.append(element) + } + } + return sequence + } +} From 823256f7160e04ad80a5c6f2b5a75576f4f2009e Mon Sep 17 00:00:00 2001 From: ibrahimoktay Date: Tue, 9 Jun 2020 21:09:21 +0300 Subject: [PATCH 4/5] Refactor short test with computed property --- Sources/ArgumentParser/Usage/UsageGenerator.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/ArgumentParser/Usage/UsageGenerator.swift b/Sources/ArgumentParser/Usage/UsageGenerator.swift index fdd3b5a83..be84a058e 100644 --- a/Sources/ArgumentParser/Usage/UsageGenerator.swift +++ b/Sources/ArgumentParser/Usage/UsageGenerator.swift @@ -118,7 +118,7 @@ extension ArgumentDefinition { } var preferredNameForSynopsis: Name? { - names.first{ $0 != .short($0.valueString.first!) } ?? names.first + names.first{ !$0.isShort } ?? names.first } var synopsisValueName: String? { From 10c5d9ea586bd5ddbaa1f2816091a0c5f43955bd Mon Sep 17 00:00:00 2001 From: ibrahimoktay Date: Tue, 9 Jun 2020 21:32:55 +0300 Subject: [PATCH 5/5] Use pattern-matching instead of equality comparison --- Sources/ArgumentParser/Parsing/Name.swift | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/Sources/ArgumentParser/Parsing/Name.swift b/Sources/ArgumentParser/Parsing/Name.swift index bb2c1b095..267d9e48d 100644 --- a/Sources/ArgumentParser/Parsing/Name.swift +++ b/Sources/ArgumentParser/Parsing/Name.swift @@ -55,7 +55,12 @@ extension Name { } var isShort: Bool { - return self == .short(self.valueString.first!) + switch self { + case .short: + return true + default: + return false + } } }