add control of exclusivity for Bool Flags when inversion is enabled (#62)

Author: glessard

Sources/ArgumentParser/Parsable Properties/Flag.swift

@@ -129,14 +129,17 @@ extension Flag where Value == Optional<Bool> {
   ///   - 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)
       })
   }
 }
@@ -248,17 +254,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
@@ -293,13 +289,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

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])
   }

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)
+//    }
   }
 }
 

Tests/UnitTests/ErrorMessageTests.swift

@@ -137,16 +137,24 @@ 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 {
   func testDuplicateFlags() {
     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\'")
   }
 }