Remove ExpressibleByArgument conformance from Optional (#173)

* Remove ExpressibleByArgument conformance for Optional

It turns out that the conditional conformance for Optional was a bad idea, and
it should be handled more like Array, with specific initializers for the Optional
case. Primarily, this is because providing a default value for an optional property
doesn't make sense -- the default is already nil, and a non-nil default means that
the property will never be nil and therefore shouldn't be optional.

* Drop duplicated argument definitions

d1 and d6 are duplicates of c2 and c, respectively.

* Correctly mark optional args/options as optional

* Correct documentation for Option/Argument

* Use the correct parameter name in the documentation

Author: natecook1000

Sources/ArgumentParser/Parsable Properties/Argument.swift

@@ -70,11 +70,9 @@ extension Argument: DecodableParsedWrapper where Value: Decodable {}
 extension Argument where Value: ExpressibleByArgument {
   /// Creates a property that reads its value from an argument.
   ///
-  /// If the property has an `Optional` type, the argument is optional and
-  /// defaults to `nil`.
-  ///
   /// - Parameters:
-  ///   - initial: A default value to use for this property.
+  ///   - initial: A default value to use for this property. If `initial` is
+  ///     `nil`, the user must supply a value for this argument.
   ///   - help: Information about how to use this argument.
   public init(
     default initial: Value? = nil,
@@ -130,6 +128,48 @@ public enum ArgumentArrayParsingStrategy {
 }
 
 extension Argument {
+  /// Creates an optional property that reads its value from an argument.
+  ///
+  /// The argument is optional for the caller of the command and defaults to 
+  /// `nil`.
+  ///
+  /// - Parameter help: Information about how to use this argument.
+  public init<T: ExpressibleByArgument>(
+    help: ArgumentHelp? = nil
+  ) where Value == T? {
+    self.init(_parsedValue: .init { key in
+      var arg = ArgumentDefinition(
+        key: key,
+        kind: .positional,
+        parsingStrategy: .nextAsValue,
+        parser: T.init(argument:),
+        default: nil)
+      arg.help.help = help
+      return ArgumentSet(arg.optional)
+    })
+  }
+  
+  @available(*, deprecated, message: """
+    Default values don't make sense for optional properties.
+    Remove the 'default' parameter if its value is nil,
+    or make your property non-optional if it's non-nil.
+    """)
+  public init<T: ExpressibleByArgument>(
+    default initial: T?,
+    help: ArgumentHelp? = nil
+  ) where Value == T? {
+    self.init(_parsedValue: .init { key in
+      ArgumentSet(
+        key: key,
+        kind: .positional,
+        parsingStrategy: .nextAsValue,
+        parseType: T.self,
+        name: .long,
+        default: initial,
+        help: help)
+    })
+  }
+  
   /// Creates a property that reads its value from an argument, parsing with
   /// the given closure.
   ///

Sources/ArgumentParser/Parsable Properties/Option.swift

@@ -72,14 +72,12 @@ extension Option: DecodableParsedWrapper where Value: Decodable {}
 extension Option where Value: ExpressibleByArgument {
   /// Creates a property that reads its value from a labeled option.
   ///
-  /// If the property has an `Optional` type, or you provide a non-`nil`
-  /// value for the `initial` parameter, specifying this option is not
-  /// required.
-  ///
   /// - Parameters:
   ///   - name: A specification for what names are allowed for this flag.
   ///   - initial: A default value to use for this property. If `initial` is
   ///     `nil`, this option and value are required from the user.
+  ///   - parsingStrategy: The behavior to use when looking for this option's
+  ///     value.
   ///   - help: Information about how to use this option.
   public init(
     name: NameSpecification = .long,
@@ -216,17 +214,66 @@ public enum ArrayParsingStrategy {
 }
 
 extension Option {
-  /// Creates a property that reads its value from a labeled option, parsing
-  /// with the given closure.
+  /// Creates a property that reads its value from a labeled option.
   ///
   /// If the property has an `Optional` type, or you provide a non-`nil`
   /// value for the `initial` parameter, specifying this option is not
   /// required.
   ///
   /// - Parameters:
   ///   - name: A specification for what names are allowed for this flag.
+  ///   - parsingStrategy: The behavior to use when looking for this option's
+  ///     value.
+  ///   - help: Information about how to use this option.
+  public init<T: ExpressibleByArgument>(
+    name: NameSpecification = .long,
+    parsing parsingStrategy: SingleValueParsingStrategy = .next,
+    help: ArgumentHelp? = nil
+  ) where Value == T? {
+    self.init(_parsedValue: .init { key in
+      var arg = ArgumentDefinition(
+        key: key,
+        kind: .name(key: key, specification: name),
+        parsingStrategy: ArgumentDefinition.ParsingStrategy(parsingStrategy),
+        parser: T.init(argument:),
+        default: nil)
+      arg.help.help = help
+      return ArgumentSet(arg.optional)
+    })
+  }
+
+  @available(*, deprecated, message: """
+    Default values don't make sense for optional properties.
+    Remove the 'default' parameter if its value is nil,
+    or make your property non-optional if it's non-nil.
+    """)
+  public init<T: ExpressibleByArgument>(
+    name: NameSpecification = .long,
+    default initial: T?,
+    parsing parsingStrategy: SingleValueParsingStrategy = .next,
+    help: ArgumentHelp? = nil
+  ) where Value == T? {
+    self.init(_parsedValue: .init { key in
+      var arg = ArgumentDefinition(
+        key: key,
+        kind: .name(key: key, specification: name),
+        parsingStrategy: ArgumentDefinition.ParsingStrategy(parsingStrategy),
+        parser: T.init(argument:),
+        default: initial)
+      arg.help.help = help
+      return ArgumentSet(arg.optional)
+    })
+  }
+
+  /// Creates a property that reads its value from a labeled option, parsing
+  /// with the given closure.
+  ///
+  /// - Parameters:
+  ///   - name: A specification for what names are allowed for this flag.
   ///   - initial: A default value to use for this property. If `initial` is
   ///     `nil`, this option and value are required from the user.
+  ///   - parsingStrategy: The behavior to use when looking for this option's
+  ///     value.
   ///   - help: Information about how to use this option.
   ///   - transform: A closure that converts a string into this property's
   ///     type or throws an error.

Sources/ArgumentParser/Parsable Types/ExpressibleByArgument.swift

@@ -33,23 +33,6 @@ extension String: ExpressibleByArgument {
   }
 }
 
-extension Optional: ExpressibleByArgument where Wrapped: ExpressibleByArgument {
-  public init?(argument: String) {
-    if let value = Wrapped(argument: argument) {
-      self = value
-    } else {
-      return nil
-    }
-  }
-  
-  public var defaultValueDescription: String {
-    guard let value = self else {
-      return "none"
-    }
-    return "\(value)"
-  }
-}
-
 extension RawRepresentable where Self: ExpressibleByArgument, RawValue: ExpressibleByArgument {
   public init?(argument: String) {
     if let value = RawValue(argument: argument) {

Sources/ArgumentParser/Parsing/ArgumentDefinition.swift

@@ -145,6 +145,7 @@ extension ArgumentDefinition: CustomDebugStringConvertible {
 extension ArgumentDefinition {
   var optional: ArgumentDefinition {
     var result = self
+    
     result.help.options.insert(.isOptional)
     return result
   }

Sources/ArgumentParser/Parsing/ArgumentSet.swift

@@ -176,7 +176,7 @@ extension ArgumentSet {
 
 extension ArgumentDefinition {
   /// Create a unary / argument that parses using the given closure.
-  fileprivate init<A>(key: InputKey, kind: ArgumentDefinition.Kind, parsingStrategy: ParsingStrategy = .nextAsValue, parser: @escaping (String) -> A?, parseType type: A.Type = A.self, default initial: A?) {
+  init<A>(key: InputKey, kind: ArgumentDefinition.Kind, parsingStrategy: ParsingStrategy = .nextAsValue, parser: @escaping (String) -> A?, parseType type: A.Type = A.self, default initial: A?) {
     let initialValueCreator: (InputOrigin, inout ParsedValues) throws -> Void
     if let initialValue = initial {
       initialValueCreator = { origin, values in

Tests/ArgumentParserEndToEndTests/SourceCompatEndToEndTests.swift

@@ -39,12 +39,10 @@ fileprivate struct AlmostAllArguments: ParsableArguments {
   @Argument(default: 0) var c2: Int?
 
   @Argument(default: 0, help: "", transform: { _ in 0 }) var d: Int?
-  @Argument(default: 0) var d1: Int?
   @Argument(help: "") var d2: Int?
   @Argument(transform: { _ in 0 }) var d3: Int?
   @Argument(help: "", transform: { _ in 0 }) var d4: Int?
   @Argument(default: 0, transform: { _ in 0 }) var d5: Int?
-  @Argument(default: 0, help: "") var d6: Int?
 
   @Argument(parsing: .remaining, help: "") var e: [Int]
   @Argument() var e0: [Int]