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

Remove ExpressibleByArgument conformance from Optional #173

Merged
merged 6 commits into from
Jun 2, 2020

Conversation

natecook1000
Copy link
Member

Description

This drops the ExpressibleByArgument conformance for Optional. This conformance doesn't quite make sense, and allowed property declarations like this:

@Option(default: 0) var count: Int?

The default value meant that count would never be nil, and therefore shouldn't be optional.

Detailed Design

This change adds initializers specifically for when an Option's or Argument's value is optional, without a default parameter as well as deprecated versions that include a default parameter to guide clients to the correct declaration syntax.

Documentation Plan

The new initializers are documented; related initializers have been updated. There is no change required in the guides.

Test Plan

Existing tests exercise the functionality and verify the largely source-compatible nature of the change.

Source Impact

While this is technically source-breaking, it should have minimal impact in regular usage. All the declaration sites for arguments and options with optional values are still supported, with deprecation warnings for declarations that combine a default value and an optional type. If a property is declared as optional but does not have a default value, there won't even be a deprecation notice.

Code that uses Optional's conformance to ExpressibleByArgument, however, will break without warning. Users can work around this by adding an Optional-specific code path, or by other means.

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

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.
d1 and d6 are duplicates of c2 and c, respectively.
@natecook1000
Copy link
Member Author

@swift-ci Please test

@natecook1000
Copy link
Member Author

@swift-ci Please test

1 similar comment
@natecook1000
Copy link
Member Author

@swift-ci Please test

@natecook1000 natecook1000 merged commit c87d0d0 into master Jun 2, 2020
@natecook1000 natecook1000 deleted the nate/optional_non_expressible branch June 2, 2020 02:47
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.

1 participant