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

Adding mapping over Results containing collections #123

Closed
wants to merge 1 commit into from
Closed

Adding mapping over Results containing collections #123

wants to merge 1 commit into from

Conversation

sam-w
Copy link

@sam-w sam-w commented Nov 29, 2015

Usecase:

let foo = self
    .iLoveResults()
    .flatMap(butAreTheseResultsGoodEnough)
    .flatMap { $0.count > 3 ? .Success($0) : .Failure(someError) } //and are there enough of them?

func iLoveResults() -> Result<[String], NSError> {
    return .Success(["aFineResult", "anotherExcellentResult", "aBadResult"])
}

func butAreTheseResultsGoodEnough(result: String) -> Result<String, NSError> {
    return (result.characters.count > 10 ? .Success(result) : .Failure(someError))
}

If I've got a Result which (on .Success) contains a Collection, I'd like to have the option to handle that success as a whole, or item-by-item with minimal boilerplate. In the example above, the first flatMap deals with the results item-by-item, the second as a whole collection. The flatMap used in each case is inferred from the context.

@robrix
Copy link
Contributor

robrix commented Nov 29, 2015

Thanks for the PR!

I’m personally 👎 on overloading map/flatMap to have distinct semantics. Rationale:

  1. Each additional overload makes it more complicated to understand any particular use of the overloaded symbol.
  2. Each additional overload makes it more complicated for Swift to infer the correct types, and therefore can break source compatibility with previous versions by triggering the dreaded Expression was too complex to be solved in reasonable time error. Just as bad, it can further compound already slow compiles.
  3. While I certainly understand the motivation, I don’t feel that the current solution, of mapping Results by a function which maps CollectionTypes, is overly onerous.
  4. There isn’t much precedent for it in stdlib. There’s only one case I can think of, the collection-of-optionals overload of flatMap, and I wish they had simply chosen a different name for it.

Overall I think I’d prefer to pass on this, but I’ll leave it open for further discussion. @antitypical/result, I’d particularly like to hear any dissenting opinions! 😄

@NachoSoto
Copy link
Contributor

I was thinking the same thing, I couldn't have put it better :D

@sam-w don't get us wrong though! What you did makes a lot of sense, but I agree with @robrix, I think this makes more sense as an internal extensions in your app rather than in the library, for those reasons.

@neilpa
Copy link
Member

neilpa commented Nov 29, 2015

I agree as well. Definitely convenient if you need it, but not enough to warrant inclusion here.

@sam-w
Copy link
Author

sam-w commented Nov 29, 2015

No worries, and thank you for the feedback all. I'll add these as internal extensions in my project :)

@sam-w sam-w closed this Nov 29, 2015
@thekarladam
Copy link

Shouldn't this be Result<[Result<String, UnitError>], NoError> which should automatically flatten down to Result<[String], UnitError> with a normal .flatten { $0.value || $0.error } as well as an easily provided .flatSuccess(), .flatError()?

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.

None yet

5 participants