Skip to content

Fix for @Option(transform:) with optional type#619

Merged
natecook1000 merged 4 commits into
mainfrom
optional-transform-ambiguity
Mar 5, 2024
Merged

Fix for @Option(transform:) with optional type#619
natecook1000 merged 4 commits into
mainfrom
optional-transform-ambiguity

Conversation

@natecook1000
Copy link
Copy Markdown
Member

@natecook1000 natecook1000 commented Feb 29, 2024

Due to the restructuring in #477, there was ambiguity between the unconstrained @Option initializer that uses a transform (but no initial value) and the one that is constrained to the property being optional. This marks the unconstrained version as disfavored, which allows overload resolution to select the optional version when appropriate.

Fixes #618.

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

Due to the restructuring in #477, there was ambiguity between the
unconstrained `@Option` initializer that uses a transform (but no
initial value) and the one that is constrained to the property being
optional. This marks the unconstrained version as disfavored, which
allows overload resolution to select the optional version when
appropriate.

Fixes #618.
@natecook1000
Copy link
Copy Markdown
Member Author

@swift-ci Please test

@Coeur
Copy link
Copy Markdown
Contributor

Coeur commented Feb 29, 2024

Thank you Nate.
To be exhaustive, we also need to fix Argument.swift by adding @_disfavoredOverload line 417, and add the same test replacing @Option with @Argument.

Copy link
Copy Markdown
Collaborator

@rauhul rauhul left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! I would've expected the previous tests I added to catch this

@natecook1000
Copy link
Copy Markdown
Member Author

@swift-ci Please test

@Coeur
Copy link
Copy Markdown
Contributor

Coeur commented Mar 1, 2024

Question: to facilitate the understanding of the documentation, should we make such below changes too?

Option.swift, Line 508, change:

  /// var foo: String = "bar"

To:

  /// var foo: String? = "bar"

Option.swift, Line 577, change:

  /// var foo: String

To:

  /// var foo: String?

@natecook1000
Copy link
Copy Markdown
Member Author

@swift-ci Please test

Comment on lines +308 to +312
/// @Option var name: String
/// ```
///
/// - Parameters:
/// - name: A specification for what names are allowed for this flag.
/// - parsingStrategy: The behavior to use when looking for this option's value.
/// - name: A specification for what names are allowed for this option.
Copy link
Copy Markdown
Contributor

@Coeur Coeur Mar 1, 2024

Choose a reason for hiding this comment

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

The rename from foo to name may add to confusion, since there is also a name parameter in the initializer.

I suggested using title like in previous example.

/// Creates a property with a default value provided by standard Swift default
/// value syntax, parsing with the given closure.
/// Creates an optional property that reads its value from a labeled option,
/// parsing with the given closure, with an explicit `nil` default.
Copy link
Copy Markdown
Contributor

@Coeur Coeur Mar 1, 2024

Choose a reason for hiding this comment

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

In general, maybe yes. But it would also work with non-nil default, like:

var char: Character? = " "

The fact that it's a mutable optional would then allow it to eventually become nil later in time (when a program wants to free some memory for instance).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That way of declaring (optional with a non-nil default) ends up using a different initializer that has a deprecation warning, since the behavior usually isn't what the author intends. I hadn't considered the use case you're describing here, but in general the ArgumentParser API is designed to essentially provide a way to write a function interface via the command line; the type declarations should match the CL interface.

/// ```swift
/// @Option()
/// var foo: [String]
/// @Option var char: [Character]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit, plural: char -> chars

Same thing on lines 748 and 796

@natecook1000
Copy link
Copy Markdown
Member Author

@swift-ci Please test

@natecook1000 natecook1000 enabled auto-merge (squash) March 5, 2024 19:16
@natecook1000 natecook1000 merged commit 7191549 into main Mar 5, 2024
@natecook1000 natecook1000 deleted the optional-transform-ambiguity branch March 5, 2024 19:20
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.

Ambiguous use of 'init(name:parsing:help:completion:transform:)' when transform throws

3 participants