Skip to content

Make Option work with Optionals#58

Merged
mdiep merged 4 commits intomasterfrom
truly-optional-options
Mar 26, 2016
Merged

Make Option work with Optionals#58
mdiep merged 4 commits intomasterfrom
truly-optional-options

Conversation

@mdiep
Copy link
Copy Markdown
Member

@mdiep mdiep commented Feb 29, 2016

In Carthage/Carthage#1143, it would be nice to use an Option<String?> for a command-line option. Since the default value isn't really known, it would be preferable to default to nil. This PR makes that possible, but it introduces a breaking change.

  • Make Option require a defaultValue. Previously, a nil default meant that the parameter was required. Carthage doesn't use this, and I think that behavior is a little silly given that it's called option. If this behavior is required somewhere, I'd rather see another type added.
  • Make a version of <| that works with an Option<T?> where T: ArgumentType.

The implementation isn't as nice as I'd like. I ran into Swift limitations, segfaults, and, I think, some type inference errors. But ultimately I arrived at something that at least works.

@ikesyo
Copy link
Copy Markdown
Member

ikesyo commented Mar 22, 2016

I will take a look in a couple of days.

@ikesyo
Copy link
Copy Markdown
Member

ikesyo commented Mar 22, 2016

Or, the non-Optional version of <| would call the Optional version. Unfortunately, those don't work.

I tried an implementation: 715684d. How do you feel about it?

@ikesyo
Copy link
Copy Markdown
Member

ikesyo commented Mar 22, 2016

Other than that change, LGTM 👍

Conflicts:
	Tests/OptionSpec.swift
@mdiep mdiep merged commit 86a80b3 into master Mar 26, 2016
@mdiep mdiep deleted the truly-optional-options branch March 26, 2016 23:54
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