Add ability to retrieve list of possible options for Option property

[bripeticca]

Previously, information regarding a custom Option type's possible values was lost when generating subsequent structures in the pipeline to describe the argument. This would largely have to be defined by the user in the discussion string property in ArgumentHelp and would provide no mechanism to directly access the allowable values for a custom Option.

This adds an extra static property on ExpressibleByArgument labelled defaultValueDescriptions, which maps the value name to its description. It is up to the user to implement the defaultValueDescription for their custom CaseIterable type to fully support this behaviour. Further refactoring has been made downstream to the ArgumentDefinition and subsequent structures describing the argument to allow the ability to retrieve the value descriptions from the ExpressibleByArgument type, making information about the property more visible and accessible.

A new enumeration ArgumentDiscussion has been introduced to better encapsulate the discussion information for an argument, and includes cases for discussion sections that contain only static text, as well as cases that include a list of argument values that derive from a type that implements ExpressibleByArgument, CaseIterable and RawRepresentable.

Addresses #637

Checklist

• I've added at least one test that validates that my change is working, if appropriate
• I've followed the code style of the rest of the project
• I've read the Contribution Guidelines
• I've updated the documentation if necessary

[cmcgee1024]

Overall this looks good to me. I've added thoughts and questions along the way. I don't feel comfortable enough with this repo to approve at this time.

[natecook1000]

Thanks so much for looking at this, @bripeticca! The updated help screens look really clear, and it's great that adoption is all opt in. I think we can cut back on the new API a little bit – if you agree, we just have a protocol name to discuss. Thanks!

[natecook1000]

This doesn't seem like the right place for a property this type, since a command generally contains a group of flags, options, and arguments. Can you say more about why CommandConfiguration should include an ArgumentDiscussion?

Separately, changing the type of discussion is source breaking – we'll need a different strategy if we do add this.

[bripeticca]

Ah, good point! I had changed this as I was making my way through the DumpHelpGenerator and updating the inits for CommandInfoV0, ArgumentInfoV0, etc. to consider ArgumentDiscussion. Along the way, I updated each struct that contained a discussion string to instead use ArgumentDiscussion - it seems like for CommandConfiguration this is unnecessary though! Since it's a breaking change, I can revert.

[natecook1000]

Same note as above, here.

[natecook1000]

If we don't use this in CommandConfiguration, it also looks like it doesn't need to be public.

[natecook1000]

This is a tricky protocol to name – I'd like to make sure we leave room for adopting this named-value feature for arguments, as well. This command type should be able to have the nice value-based help text for the color argument:

struct Example: ParsableCommand {
    @Argument var color: Color
}

We don't need to land @Argument support for this, but we should be able to make room for it.

[bripeticca]

That's a great point! The naming is definitely tricky :) Perhaps something a little more generic like EnumerableValue could be used instead?

[natecook1000]

Since Value.self ends up getting erased into (any EnumerableOptionValue)? anyway, I wonder if we can skip all these overloads and just try casting the type in the regular ExpressibleByArgument initializers.

[bripeticca]

For sure - I just tried this out locally by adding the following to the beginning of each ExpressibleByArgument init:

var help = help
if let type = Value.self as? (any EnumerableOptionValue.Type) {
  help = ArgumentHelp.init(
  help?.abstract ?? "",
  discussion: help?.discussion,
  options: type, // retaining necessary type information for enumerable values here
  valueName: help?.valueName,
  visibility: help?.visibility ?? .default
  )
}

This way we can still retain the modified ArgumentHelp for types implementing EnumerableOptionValue (or whatever we decide to rename it to!).

Alternatively, I can modify the ArgumentHelp init to take the options parameter and have it be optional, and can remove some code so it comes down to this:

// ArgumentHelp init:
public init(
  _ abstract: String = "",
  discussion: String? = nil,
  valueName: String? = nil,
  options: (any EnumerableOptionValue.Type)? = nil, // made this optional
  visibility: ArgumentVisibility = .default)
{
  self.abstract = abstract
  self.discussion = discussion
  self.valueName = valueName
  self.options = options
  self.visibility = visibility
} 

and the resulting ExpressibleByArgument inits would have some additional code that looks something like this:

self.init(_parsedValue: .init { key in
  let arg = ArgumentDefinition(
    container: Bare<Value>.self,
    key: key,
    kind: .name(key: key, specification: name),
    help: .init(
      help?.abstract ?? "",
      discussion: help?.discussion,
      valueName: help?.valueName,
      options: Value.self as? (any EnumerableOptionValue.Type),
      visibility: help?.visibility ?? .default
    ),
    parsingStrategy: parsingStrategy.base,
    initial: wrappedValue,
    completion: completion)

  return ArgumentSet(arg)
})

If this seems good to you, I can go ahead and change it!

[natecook1000]

Sorry for the delay here! I've been wrestling with whether we can avoid the EnumerableOptionValue protocol altogether, since it seems to be a refinement of the ExpressibleByArgument protocol that we're already handling for a couple slightly different uses. In particular, adding features like this one to the library is challenging in that we need to design a source-compatible addition that makes some kind of intuitive sense with the features that are already present.

The most related existing feature supported by the ExpressibleByArgument protocol is the ability to automatically include value names in the help description when a type is CaseIterable. Since the change in this PR kind of augments that to include descriptions as well as value names, I think we can piggy back on that existing design.

In particular, what would you think about adding another customization point to ExpressibleByArgument:

public protocol ExpressibleByArgument {
  // ...other members...
  
  // Existing static var for value strings:

  /// An array of all possible strings that can convert to a value of this
  /// type, for display in the help screen.
  ///
  /// The default implementation of this property returns an empty array. If the
  /// conforming type is also `CaseIterable`, the default implementation returns
  /// an array with a value for each case.
  static var allValueStrings: [String] { get }

  // New static var for value descriptions:
  
  static var allValueDescriptions: [String: String] { get }
}

We could provide a default implementation that returns an empty dictionary, and then any type that wants to provide a more expanded help screen could implement allValueDescriptions to provide corresponding descriptions for each element of the allValueStrings array. Internally, we can use similar machinery to capture those descriptions for use when displaying the help screen or generating the JSON help description.

[bripeticca]

This is a great idea! Thanks for the feedback - I tried to implement this locally and it seems to work well.

[bripeticca]

@swift-ci please test

[bripeticca]

@swift-ci please test

[bripeticca]

@swift-ci please test

[natecook1000]

This is looking like the right API! Just a couple of nits and then a question about testing - should be good to go after that!

[natecook1000]

@rauhul Do we need to update the tool info version for a change like this?

[rauhul]

Yeah this will need a version bump and will be source break. Thats not a deal breaker, but can be annoying.

Alternatively we can call this discussion2 and deprecate the old one.

[bripeticca]

I'll go ahead and rename the property in that case! Should this deprecation strategy also be applied to ArgumentInfoV0?

[bripeticca]

@swift-ci please test

[rauhul]

Is there a real need for "Values:"? I dont think I like the difference in indentation with/without a "discussion"

[bripeticca]

No real need, I can change it!

[rauhul]

Can we factor this body out into a Discussion type and use it the 4 places changed here?

[bripeticca]

I've added a DiscussionText struct to the project that encapsulates this logic!

[bripeticca]

@swift-ci please test

[rauhul]

Im a little confused by how this list view interacts with the change introduced in #594. Do you have a test that covers both features together?

[bripeticca]

I can add a test that demonstrates the differences between both of these features!

The idea is that if the type for the @Option property is an enum that implements ExpressibleByArgument and if the enum also implements the defaultValueDescription property for its cases, then the generated help text will enumerate each of the enum's values with the description defined in defaultValueDescription.

For example:

enum OptionEnumerated: String, CaseIterable, ExpressibleByArgument { 
    case one
    case two
    case three

    public var defaultValueDescription: String { 
        switch self { 
            case .one:
                return "The number one."
            case .two:
                return "The number two."
            case .three:
                return "The number three."
        }
    }
}

struct MyCommand: ParsableCommand { 
    @Option(help: "The values for this option should be enumerated with a description.")
    var list: OptionEnumerated
}

// Example generated help text:
...
--list <list>         The values for this option should be enumerated with a description.
      one              - The number one.
      two              - The number two.
      three            - The number three.
...

Otherwise if this property is not implemented in the enum, then it will generate the help text like so:

enum OptionWithoutEnumeratedHelpText: String, CaseIterable, ExpressibleByArgument { 
    case one = "1"
    case two = "2"
    case three = "3"
}

struct MyOtherCommand: ParsableCommand { 
    @Option(help: "This is an option without explicit enumeration in the help text.")
    var values: OptionWithoutEnumeratedHelpText
}

// Example generated help text:
...
 --values <values>       This is an option without explicit enumeration in the
                                       help text. (values: 1, 2, 3)
...

[bripeticca]

@swift-ci please test

[bripeticca]

@swift-ci please test

[rauhul]

I'm pretty happy with this change! If @natecook1000 is happy with it, we can merge :)

[natecook1000]

@swift-ci Please test

[natecook1000]

@swift-ci Please test

[natecook1000]

Thanks so much for your patience on this, @bripeticca! 🎉🎉🎉