diff --git a/Sources/ArgumentParser/Parsable Properties/Flag.swift b/Sources/ArgumentParser/Parsable Properties/Flag.swift index 67f97f131..1fa0c1ab6 100644 --- a/Sources/ArgumentParser/Parsable Properties/Flag.swift +++ b/Sources/ArgumentParser/Parsable Properties/Flag.swift @@ -129,14 +129,17 @@ extension Flag where Value == Optional { /// - name: A specification for what names are allowed for this flag. /// - inversion: The method for converting this flags name into an on/off /// pair. + /// - exclusivity: The behavior to use when an on/off pair of flags is + /// specified. /// - help: Information about how to use this flag. public init( name: NameSpecification = .long, inversion: FlagInversion, + exclusivity: FlagExclusivity = .chooseLast, help: ArgumentHelp? = nil ) { self.init(_parsedValue: .init { key in - .flag(key: key, name: name, default: nil, inversion: inversion, help: help) + .flag(key: key, name: name, default: nil, inversion: inversion, exclusivity: exclusivity, help: help) }) } } @@ -188,15 +191,18 @@ extension Flag where Value == Bool { /// - initial: The default value for this flag. /// - inversion: The method for converting this flag's name into an on/off /// pair. + /// - exclusivity: The behavior to use when an on/off pair of flags is + /// specified. /// - help: Information about how to use this flag. public init( name: NameSpecification = .long, default initial: Bool? = false, inversion: FlagInversion, + exclusivity: FlagExclusivity = .chooseLast, help: ArgumentHelp? = nil ) { self.init(_parsedValue: .init { key in - .flag(key: key, name: name, default: initial, inversion: inversion, help: help) + .flag(key: key, name: name, default: initial, inversion: inversion, exclusivity: exclusivity, help: help) }) } } @@ -247,17 +253,7 @@ extension Flag where Value: CaseIterable, Value: RawRepresentable, Value.RawValu return ArgumentDefinition.flag(name: name, key: key, caseKey: caseKey, help: help, parsingStrategy: .nextAsValue, initialValue: initial, update: .nullary({ (origin, name, values) in // TODO: We should catch duplicate flags that hit a single part of // an exclusive argument set in the value parsing, not here. - let previous = values.element(forKey: key) - switch (hasUpdated, previous, exclusivity) { - case (true, let p?, .exclusive): - // This value has already been set. - throw ParserError.duplicateExclusiveValues(previous: p.inputOrigin, duplicate: origin, originalInput: values.originalInput) - case (false, _, _), (_, _, .chooseLast): - values.set(value, forKey: key, inputOrigin: origin) - default: - break - } - hasUpdated = true + hasUpdated = try ArgumentSet.updateFlag(key: key, value: value, origin: origin, values: &values, hasUpdated: hasUpdated, exclusivity: exclusivity) })) } return exclusivity == .exclusive @@ -292,13 +288,9 @@ extension Flag { let caseKey = InputKey(rawValue: value.rawValue) let help = ArgumentDefinition.Help(options: .isOptional, help: help, key: key) return ArgumentDefinition.flag(name: name, key: key, caseKey: caseKey, help: help, parsingStrategy: .nextAsValue, initialValue: nil as Element?, update: .nullary({ (origin, name, values) in - if hasUpdated && exclusivity == .exclusive { - throw ParserError.unexpectedExtraValues([(origin, String(describing: value))]) - } - if !hasUpdated || exclusivity == .chooseLast { - values.set(value, forKey: key, inputOrigin: origin) - } - hasUpdated = true + // TODO: We should catch duplicate flags that hit a single part of + // an exclusive argument set in the value parsing, not here. + hasUpdated = try ArgumentSet.updateFlag(key: key, value: value, origin: origin, values: &values, hasUpdated: hasUpdated, exclusivity: exclusivity) })) } return exclusivity == .exclusive diff --git a/Sources/ArgumentParser/Parsing/ArgumentSet.swift b/Sources/ArgumentParser/Parsing/ArgumentSet.swift index a3b41cf73..976f8223d 100644 --- a/Sources/ArgumentParser/Parsing/ArgumentSet.swift +++ b/Sources/ArgumentParser/Parsing/ArgumentSet.swift @@ -169,24 +169,39 @@ extension ArgumentSet { }) return ArgumentSet(arg) } + + static func updateFlag(key: InputKey, value: Any, origin: InputOrigin, values: inout ParsedValues, hasUpdated: Bool, exclusivity: FlagExclusivity) throws -> Bool { + let previous = values.element(forKey: key) + switch (hasUpdated, previous, exclusivity) { + case(true, let previous?, .exclusive): + // This value has already been set. + throw ParserError.duplicateExclusiveValues(previous: previous.inputOrigin, duplicate: origin, originalInput: values.originalInput) + case (false, _, _), (_, _, .chooseLast): + values.set(value, forKey: key, inputOrigin: origin) + return true + default: + return hasUpdated + } + } /// Creates an argument set for a pair of inverted Boolean flags. - static func flag(key: InputKey, name: NameSpecification, default initialValue: Bool?, inversion: FlagInversion, help: ArgumentHelp?) -> ArgumentSet { + static func flag(key: InputKey, name: NameSpecification, default initialValue: Bool?, inversion: FlagInversion, exclusivity: FlagExclusivity, help: ArgumentHelp?) -> ArgumentSet { // The flag is required if initialValue is `nil`, otherwise it's optional let helpOptions: ArgumentDefinition.Help.Options = initialValue != nil ? .isOptional : [] let help = ArgumentDefinition.Help(options: helpOptions, help: help, defaultValue: initialValue.map(String.init), key: key) let (enableNames, disableNames) = inversion.enableDisableNamePair(for: key, name: name) - - let enableArg = ArgumentDefinition(kind: .named(enableNames), help: help, update: .nullary({ (origin, name, values) in - values.set(true, forKey: key, inputOrigin: origin) + + var hasUpdated = false + let enableArg = ArgumentDefinition(kind: .named(enableNames),help: help, update: .nullary({ (origin, name, values) in + hasUpdated = try ArgumentSet.updateFlag(key: key, value: true, origin: origin, values: &values, hasUpdated: hasUpdated, exclusivity: exclusivity) }), initial: { origin, values in if let initialValue = initialValue { values.set(initialValue, forKey: key, inputOrigin: origin) } }) let disableArg = ArgumentDefinition(kind: .named(disableNames), help: ArgumentDefinition.Help(options: [.isOptional], key: key), update: .nullary({ (origin, name, values) in - values.set(false, forKey: key, inputOrigin: origin) + hasUpdated = try ArgumentSet.updateFlag(key: key, value: false, origin: origin, values: &values, hasUpdated: hasUpdated, exclusivity: exclusivity) }), initial: { _, _ in }) return ArgumentSet(exclusive: [enableArg, disableArg]) } diff --git a/Tests/EndToEndTests/FlagsEndToEndTests.swift b/Tests/EndToEndTests/FlagsEndToEndTests.swift index 9ab9661c3..18d5c48ad 100644 --- a/Tests/EndToEndTests/FlagsEndToEndTests.swift +++ b/Tests/EndToEndTests/FlagsEndToEndTests.swift @@ -27,6 +27,9 @@ fileprivate struct Bar: ParsableArguments { @Flag(inversion: .prefixedNo) var extattr2: Bool? + + @Flag(inversion: .prefixedEnableDisable, exclusivity: .chooseFirst) + var logging: Bool } extension FlagsEndToEndTests { @@ -74,6 +77,13 @@ extension FlagsEndToEndTests { AssertParse(Bar.self, ["--extattr", "--no-extattr", "--extattr"]) { options in XCTAssertEqual(options.extattr, true) } + AssertParse(Bar.self, ["--enable-logging"]) { options in + XCTAssertEqual(options.logging, true) + } +// Can't test this yet, because .chooseFirst flags don't work +// AssertParse(Bar.self, ["--enable-logging", "--disable-logging"]) { options in +// XCTAssertEqual(options.logging, false) +// } } } diff --git a/Tests/UnitTests/ErrorMessageTests.swift b/Tests/UnitTests/ErrorMessageTests.swift index 4713abfb2..ec9ca7157 100644 --- a/Tests/UnitTests/ErrorMessageTests.swift +++ b/Tests/UnitTests/ErrorMessageTests.swift @@ -137,10 +137,14 @@ extension ErrorMessageTests { private enum OutputBehaviour: String, CaseIterable { case stats, count, list } private struct Options: ParsableArguments { - @Flag(name: .shortAndLong, help: "Program output") + @Flag(name: .shortAndLong, default: .list, help: "Program output") var behaviour: OutputBehaviour - @Flag() var bool: Bool + @Flag(inversion: .prefixedNo, exclusivity: .exclusive) var bool: Bool +} +private struct OptOptions: ParsableArguments { + @Flag(name: .short, help: "Program output") + var behaviour: OutputBehaviour? } extension ErrorMessageTests { @@ -148,5 +152,9 @@ extension ErrorMessageTests { AssertErrorMessage(Options.self, ["--list", "--bool", "-s"], "Value to be set with flag \'-s\' had already been set with flag \'--list\'") AssertErrorMessage(Options.self, ["-cbl"], "Value to be set with flag \'l\' in \'-cbl\' had already been set with flag \'c\' in \'-cbl\'") AssertErrorMessage(Options.self, ["-bc", "--stats", "-l"], "Value to be set with flag \'--stats\' had already been set with flag \'c\' in \'-bc\'") + + AssertErrorMessage(Options.self, ["--no-bool", "--bool"], "Value to be set with flag \'--bool\' had already been set with flag \'--no-bool\'") + + AssertErrorMessage(OptOptions.self, ["-cbl"], "Value to be set with flag \'l\' in \'-cbl\' had already been set with flag \'c\' in \'-cbl\'") } }