Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for Void? flags #191

Closed
wants to merge 1 commit into from
Closed

Add support for Void? flags #191

wants to merge 1 commit into from

Conversation

MPLew-is
Copy link
Contributor

This is intended to replace the Bool flag without an inversion, as it more closely matches the underlying usage model of a flag being provided vs. not provided on the command line.

This is mostly a proof-of-concept proposal as a discussion point, relating to the discussion about no-inversion Flags in #170. As a summary, this proposal is intended to address the behavior of flags without inversions:

struct SomeArguments: ParsableCommand {
    @Flag() var someFlag: Bool
    func run() throws {
        if someFlag {
            ...
        }
    }
}

As discussed in the comment linked above, in my opinion, a Bool doesn't represent the high-level usage model as well as an Optional type, especially with the usage of Bools in flags that have inversions and can be set by the user to have both true and false as a value, not only true. This PR proposes to replace the above usage with:

struct SomeArguments: ParsableCommand {
    @Flag() var someFlag: Void?
    func run() throws {
        if someFlag != nil {
            ...
        }
    }
}

This gains us the following benefits:

  • Sidesteps the potential confusion over a non-optional value having an implicit default value currently on master
  • Sidesteps the need to force users to provide an explicit false for every flag as currently on Allow normal Swift default property initialization syntax #170, while doing so in a Swift-like manner (since optionals already default to nil, users will be familiar with this usage pattern)
  • As mentioned, better represents the high-level usage of the command-line arguments, which is both a little cleaner and will probably help stave off future issues down the road (at least in my own experience, aligning the usage and the underlying model helps to prevent weird corner-cases as things are extended)
  • Simplifies code that deals with flags with inversions, as the inversion can now be inferred from the type and defaulted to .prefixedNo for all Bools

However, this does come at a cost:

  • Using Void? may seem strange to users that aren't "cracking the hood" of this framework, so to speak, and will likely cause some confusion that needs to be sorted out in documentation
    • This cost could be reduced by using a better-named typealias or single-case enum as described in the initial discussion linked above, but I don't know of a great name for that either (Unary?)
  • Adds some minor complexity to existing code (the addition of != nil above)

I personally feel like those benefits are worth it, but I don't think there's an exceptionally strong case for it either like there was for the default property initialization syntax in #170. Mostly, I was just curious as to what it would take to implement this and provide a concrete implementation to start off dicussion.

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

This is intended to replace the `Bool` flag without an inversion, as it more closely matches the underlying usage model of a flag being provided vs. not provided on the command line.
@@ -104,6 +104,14 @@ extension ArgumentSet {
return ArgumentSet(arg)
}

static func flag(key: InputKey, name: NameSpecification, help: ArgumentHelp?, value: Void) -> ArgumentSet {
Copy link
Contributor Author

@MPLew-is MPLew-is Jun 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also like to note that the value: Void argument is temporary, as the conflicting Bool-related flag method changes signature in #170, and this function would be flag(key:name:help:) after that.

@MPLew-is
Copy link
Contributor Author

Another side note is that my original idea here was to have a computed Bool property as the flag’s projectedValue, but could not figure out any way to make that work in a way that only affected Void? flags (you can’t add a projected value from an extension, and even doing some indirection like passing a closure through a new constructor that’s then used to determine the projected value did not seem to actually invoke that closure). If you know of some way to make the projected value work, I’d love to know that for future reference.

I also know the projected value would interfere with #128, but again this is mostly just first stab at something for the purposes of discussion.

@natecook1000
Copy link
Member

I don't think this is a change that we'll want to take. Bool and Void? are largely isomorphic, and since Bool is the currency type, we'll want to stick with that for the primary flag use case. That said, I appreciate the exploration! It does model the user input more precisely than using a Boolean.

@MPLew-is MPLew-is closed this Jul 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants