Closed sgtsquiggs closed 5 years ago
Thanks for the contrib Matthew :) Do you have any practical use case where you needed this? I don't feel super comfortable breaking public interface that way (specifically for filterMap which is being used quite heavily by people). If we do this it will have to be with some deprecation fix-it. Not sure if its worth it. Thoughts?
~Wouldn't this be backwards compatible? Every existing transform function w/o throw
would still work.~
I guess it wouldn't be backwards compatible usages when the transform method is provided as a reference. Could be added as an overload and deprecate the old as you mentioned.
I have use cases for map
throwing errors (map-with-error isn't that uncommon is it?). If I want to filter
in addition (and easily w/ filterMap
) I have to instead filter
first then map
the remaining. This is especially annoying if I'm using a switch statement since the states I filter
'd to error now exist in my map but those cases will never fire.
A simplified example:
struct SomeThirdPartyConnectionStatusEnum {
case connecting
case connected
case disconnecting
case disconnected
case errored(Error)
}
connectionStatusObservable
.map { status -> Bool? in
switch status {
case .connected:
return true
case .disconnected:
return false
case let .error(error):
throw error
default:
return nil
}
.filterNil()
vs
connectionStatusObservable
.filterMap { status -> FilterMap<Bool> in
switch status {
case .connected:
return .map(true)
case .disconnected:
return .map(false)
case let .error(error):
throw error
default:
return .ignore
}
I would be OK adding this with a proper deprecation warning, and probably a version bump as well.
Since the new method will be an overload of the existing method there will be a "fun" issue with deprecation. All non-throwing closure blocks will by default use the deprecated non-throwing method, resulting in false-positives. 😑
Ick. 😢
So instead, I added overloads. Definitely not the "best" solution, but it gets this PR in a somewhat "acceptable" state.
Let me sleep on this :) I'll take a look in the next few days.
Thanks !
@freak4pc what do you think about it? :)
Yeah, it looks good. I'm a bit worried about overloads not working well, so we might want to cover this with tests and of course rebase on top of the latest version.
I will communicate with @sgtsquiggs and if he doesn't mind will work on that by myself
Feel free to continue the good work 😎
Thanks :)
Closing cause changes were already merged
RxSwift's map, filter, and flatMap all catch (and transmit) errors via
throw
, so I have added this functionality to flatMapSync and filterMap.