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

Add map(to:) operator #601

Merged
merged 12 commits into from
Feb 6, 2018
Merged

Conversation

ra1028
Copy link
Contributor

@ra1028 ra1028 commented Feb 6, 2018

Refer #597

For example, use as below.

let (signal, observer) = Signal<String, NoError>.pipe()
let triggerSignal = signal.map(to: ())

triggerSignal.observeValues {
    print("trigger `value` events")
}

Discassion

I want to add @autoclosure @escaping, but occur an error(Ambiguous reference to member 'map) on overload map.
So, I propose to name it replace(to:), like below.

public func replace<U>(to value: @autoclosure @escaping () -> U) -> Signal<U, Error> {
    return map { _ in value() }
}

Checklist

  • Updated CHANGELOG.md.

/// - value: A new value.
///
/// - returns: A signal that will send new values.
public func map<U>(to value: U) -> Signal<U, Error> {
Copy link
Member

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?

Copy link
Contributor Author

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.

@NachoSoto
Copy link
Member

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.

/// - value: A new value.
///
/// - returns: A signal that will send new values.
public func map<U>(to value: U) -> Signal<U, Error> {
Copy link
Contributor

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.

///
/// - returns: A property that holds a mapped value from `self`.
public func map<U>(value: U) -> Property<U> {
return map { _ in value }
Copy link
Member

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?

/// - 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 }
Copy link
Member

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?

Copy link
Contributor Author

@ra1028 ra1028 Feb 6, 2018

Choose a reason for hiding this comment

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

done 👍

@NachoSoto NachoSoto merged commit a8b31f1 into ReactiveCocoa:master Feb 6, 2018
@NachoSoto
Copy link
Member

🎉

@ra1028 ra1028 deleted the feature/map-to branch February 6, 2018 20:08
@mishagray mishagray mentioned this pull request Feb 12, 2018
11 tasks
@NachoSoto
Copy link
Member

NachoSoto commented Feb 13, 2018

@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.

@mishagray
Copy link
Contributor

This PR breaks some of my existing 'map' code.

        // I have a Signal Producer of UIImage values
	let fetchImage = SignalProducer<UIImage, NoError>(value: UIImage())

        // optionalFetch SHOULD resolve as  SignalProducer<UIImage?, NoError>
	let optionalFetch = fetchImage.map { $0 as UIImage? }

	assert(optionalFetch is SignalProducer<UIImage?, NoError>, "did I get the right type?") 
        // The line above actually gives the compiler warning: "Cast from 'SignalProducer<(UIImage?) -> UIImage?, NoError>' to unrelated type 'SignalProducer<UIImage?, NoError>' always fails"
	assert(!(optionalFetch is SignalProducer<(UIImage?) -> UIImage?, NoError>), "did I get the wrong type?")
       // line above generates compiler warning: "'is' test is always true"

       // Defining the type seems to get the compiler to 'pick' the right version of 'map'.   But this kind of ugly.
	let optionalFetch1: SignalProducer<UIImage?, NoError>  = fetchImage.map { $0 as UIImage? }
	let optionalFetch2: SignalProducer<(UIImage?) -> UIImage?, NoError>  = fetchImage.map { $0 as UIImage? }
       	assert(optionalFetch1 is SignalProducer<UIImage?, NoError>, "did I get the right type?") 
 


@NachoSoto
Copy link
Member

I'm proposing renaming this replacingValues(to:) to avoid the bad type inference due to the overload.

@mdiep
Copy link
Contributor

mdiep commented Feb 13, 2018

Wat! That does not seem like a valid use of trailing closure syntax. 😕

@NachoSoto
Copy link
Member

But Swift doesn't know that :(

@NachoSoto
Copy link
Member

Oh wait, I see what you're saying! That seems like a Swift bug then.

@mdiep
Copy link
Contributor

mdiep commented Feb 13, 2018

Yes, about to file an issue.

@NachoSoto
Copy link
Member

screen shot 2018-02-12 at 4 42 37 pm

@mdiep
Copy link
Contributor

mdiep commented Feb 13, 2018

@ra1028
Copy link
Contributor Author

ra1028 commented Feb 13, 2018

Oh... 🙇

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

4 participants