Skip to content

Conversation

kaphacius
Copy link
Contributor

This PR adds a new operator catchToResult which allows transforming a Publisher that has a specific Failure type (not Never) into a publisher that outputs Output and Failure wrapped in a Result.
A common use-case for this is when an interface should produce publishers of type AnyPublisher<Result<T, Error>, Never>, but the underlying code works with publishers like AnyPublisher<T, Error> (URLSession.DataTaskPublisher).

The opposite situation, going from AnyPublisher<Result<T, Error>, Never> to AnyPublisher<T, Error> also happens. If there is an interest in this PR I can open another one with an operator that allows doing that.

@codecov
Copy link

codecov bot commented Mar 5, 2021

Codecov Report

Merging #79 (5314a9d) into main (5b8a0c0) will increase coverage by 0.09%.
The diff coverage is 100.00%.

❗ Current head 5314a9d differs from pull request most recent head bea35a1. Consider uploading reports for the commit bea35a1 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main      #79      +/-   ##
==========================================
+ Coverage   97.01%   97.10%   +0.09%     
==========================================
  Files          60       62       +2     
  Lines        3215     3315     +100     
==========================================
+ Hits         3119     3219     +100     
  Misses         96       96              
Impacted Files Coverage Δ
Sources/Operators/MapToResult.swift 100.00% <100.00%> (ø)
Tests/MapToResultTests.swift 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b8a0c0...bea35a1. Read the comment docs.

Copy link

@manmal manmal left a comment

Choose a reason for hiding this comment

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

I think this looks great, thanks for taking up the idea!

@jasdev
Copy link
Member

jasdev commented Mar 5, 2021

Would love to see this land, thanks @kaphacius! I use TCA’s catchToEffect analog often. ✅

The opposite situation, going from AnyPublisher<Result<Output, Failure>, Never> to AnyPublisher<Output, Failure> also happens. If there is an interest in this PR I can open another one with an operator that allows doing that.

+1.

(and as a heads up, we’ll probs need some quick docs. in the README.md, for when you get a chance. 🙏🏽)

Copy link
Member

@freak4pc freak4pc left a comment

Choose a reason for hiding this comment

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

Thanks! Tests look good overall, but there are some formatting issues, for example usage of unneeded parentheses, using keywords like catch and throw as functions (e.g. catch (let e), throw(MyError.error).

If you'll clean up these things I'll be happy to merge.

Thank you!

@kaphacius
Copy link
Contributor Author

Added info to README.md and applied all of the suggested comments 👍

@jshier
Copy link

jshier commented Mar 15, 2021

Is there any particular reason not to make the name of this something more general, like toResult? It's not strictly a catching operator, as it transforms the success case as well.

(I have this operator in my code base as asResult(), though I'm not particular on as vs. to.)

@freak4pc
Copy link
Member

I agree about @jshier's remarks and this is the biggest gripe I have here. Something like maoToResult or something else makes more sense, but there's "prior art" with TCA's catchToEffect. Not sure that's really a strong enough reason to not give it our own naming.

@kaphacius
Copy link
Contributor Author

I don't have any attachment to the name to be honest, I think mapToResult sounds equally good. I'll make the changes and update the PR

@kaphacius kaphacius changed the title Add catchToResult operator, tests Add mapToResult operator, tests Mar 22, 2021
@kaphacius
Copy link
Contributor Author

Renamed the operator, tests and the PR name :)

Copy link
Member

@freak4pc freak4pc left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you :)

@freak4pc freak4pc merged commit 8a070de into CombineCommunity:main Mar 22, 2021
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.

5 participants