-
Notifications
You must be signed in to change notification settings - Fork 430
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
Add map(to:) operator #601
Conversation
Sources/Signal.swift
Outdated
/// - value: A new value. | ||
/// | ||
/// - returns: A signal that will send new values. | ||
public func map<U>(to value: U) -> Signal<U, Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add this to SignalProducer
too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, I forgot
will add it.
I think eager evaluation is precisely what we want. If somebody needs lazy evaluation of the value they can make that explicit by using the closure variant. |
Sources/Signal.swift
Outdated
/// - value: A new value. | ||
/// | ||
/// - returns: A signal that will send new values. | ||
public func map<U>(to value: U) -> Signal<U, Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should call this map(value:)
for consistency. This would match SignalProducer(value:)
, but it would also match things like skip(value:)
, filter(value:)
, which wouldn't make sense with to:
. We could also conceivably add mapError(value:)
, map(error:)
and other things.
Sources/Property.swift
Outdated
/// | ||
/// - returns: A property that holds a mapped value from `self`. | ||
public func map<U>(value: U) -> Property<U> { | ||
return map { _ in value } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you implement this using lift
as well?
Sources/SignalProducer.swift
Outdated
/// - returns: A signal producer that, when started, will send a mapped | ||
/// value of `self`. | ||
public func map<U>(value: U) -> SignalProducer<U, Error> { | ||
return map { _ in value } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you implement this using lift
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done 👍
🎉 |
@mishagray realized that this produced a regression. I wrote a test case that reproduces it: let (signal, observer) = Signal<Int, NoError>.pipe()
let mappedSignal = signal.map { $0 as Int? }
// Cannot convert value of type 'Signal<(Int?) -> Int?, NoError>' to specified type 'Signal<Int?, NoError>'
let expectedType: Signal<Int?, NoError> = mappedSignal I'm sending a PR to fix this. |
This PR breaks some of my existing 'map' code.
|
I'm proposing renaming this |
Wat! That does not seem like a valid use of trailing closure syntax. 😕 |
But Swift doesn't know that :( |
Oh wait, I see what you're saying! That seems like a Swift bug then. |
Yes, about to file an issue. |
Oh... 🙇 |
Refer #597
For example, use as below.
Discassion
I want to add
@autoclosure @escaping
, but occur an error(Ambiguous reference to member 'map
) on overloadmap
.So, I propose to name it
replace(to:)
, like below.Checklist