Skip to content

Conversation

kaphacius
Copy link
Contributor

This PR adds a new operator liftErrorFromResult which allows transforming a Publisher that outputs values wrapped in a Result<T, E> and Failure of Never to a new publisher AnyPublisher<T,E>.
A common use-case for this is when the we expect to work with Non-Never publishers, but the underlying code is producing publishers that have Output and Failure wrapped in a Result

This is a continuation (and an inverse operator) of #79

@codecov
Copy link

codecov bot commented Mar 12, 2021

Codecov Report

Merging #82 (feb7fd4) into main (5b8a0c0) will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #82      +/-   ##
==========================================
+ Coverage   97.01%   97.06%   +0.05%     
==========================================
  Files          60       62       +2     
  Lines        3215     3270      +55     
==========================================
+ Hits         3119     3174      +55     
  Misses         96       96              
Impacted Files Coverage Δ
Sources/Operators/LiftErrorFromResult.swift 100.00% <100.00%> (ø)
Tests/LiftErrorFromResultTests.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...dfc916a. Read the comment docs.

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 for the contribution! Next time it's better to open an issue to discuss first.

I'm not sure I 100% see the value of this operator:

  1. It kinda provides the same value of materialize() and dematerialize()
  2. It seems to cater for a very narrow use case
  3. It seems simple enough to add to the codebase of someone who needs it specifically for their use case.

Do other folks think this is a worthwhile addition? I'm a bit divided on it.

Transforming a stream of values to stream of results seems reasonable but I'm not sure the other way around does. @jasdev @manmal

@manmal
Copy link

manmal commented Mar 12, 2021

I agree that this operator would probably not be used as often as its inverse. Personally, I've had something like this operator in one or two projects so far. But I think it's valuable to reject or postpone new features if they are not often used. I really don't have a strong opinion on this one.

@jasdev
Copy link
Member

jasdev commented Mar 12, 2021

No strong opinion — it’s nice for sake of completeness to offer operators for both directions, but if catchToResult is used more widely, let’s maybe start there and revisit if folks express a need for this. ✅

@kaphacius
Copy link
Contributor Author

Alright, since there is no strong support for this, I'll close it. Maybe let's revisit it in the future :)

@kaphacius kaphacius closed this Mar 13, 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.

4 participants