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 prefix(value:) #2941

Merged
merged 2 commits into from
May 30, 2016
Merged

Add prefix(value:) #2941

merged 2 commits into from
May 30, 2016

Conversation

inamiy
Copy link
Contributor

@inamiy inamiy commented May 26, 2016

As discussed in Slack channel recently, there seem to have many reinvention of beginWith (old - (instancetype)startWith:(id)value), so I think it's good time to add one for Swift code.

Example of beginWith are:


2016/05/31 Updated: Renamed beginWith() to prefix(value:)

@mdiep
Copy link
Contributor

mdiep commented May 28, 2016

Two bits of feedback:

  1. This operator is usually called startWith.
  2. A Signal variant should also be added.

I agree that it can be useful and would be 👍 to adding it once those are addressed.

@inamiy
Copy link
Contributor Author

inamiy commented May 28, 2016

@mdiep Thank you for feedback!

This operator is usually called startWith.

The name startWithXXX is already used for activating SignalProducer, so I think we need to rename old - (instancetype)startWith:(id)value) operator to separate their meanings.

A Signal variant should also be added.

There will be no Signal variant because the initial value of beginWith will only be emitted via cold SignalProducer's startWithSignal() but not for hot Signal where initial value will be immediately emitted before signal.observe().

@mdiep
Copy link
Contributor

mdiep commented May 28, 2016

The name startWithXXX is already used for activating SignalProducer, so I think we need to rename old - (instancetype)startWith:(id)value) operator to separate their meanings.

I think that actually strengthens the case for naming it startWith—it reinforces that the value is sent when you call a start… method.

There will be no Signal variant because the initial value of beginWith will only be emitted via cold SignalProducer's startWithSignal() but not for hot Signal where initial value will be immediately emitted before signal.observe().

Oh, right. 😆😅

@inamiy
Copy link
Contributor Author

inamiy commented May 29, 2016

I think that actually strengthens the case for naming it startWith—it reinforces that the value is sent when you call a start… method.

Though there's a good naming reinforcement, I'm still a bit afraid to adopt this idea because there are already a lot of startWithXXX() methods and they can be easily mixed up with new startWith() operator. For example, we must always say "startWith() does not actually start SignalProducer".

As seen in slack discussion, beginWith sounds a good alternative, and this was indeed what I also come up with exact same name in Swift example.

I will +1 to the name beginWith once more.
(but no problem to rename if many people opposed 😉)

@mdiep
Copy link
Contributor

mdiep commented May 29, 2016

For example, we must always say "startWith() does not actually start SignalProducer".

The compiler will say that for us because of @warn_unused_result. ☺️

@RuiAAPeres
Copy link
Member

I am going with beginsWith as well. startXXX() returns a disposable, where the potential startsWith wouldnt.

@mdiep
Copy link
Contributor

mdiep commented May 30, 2016

@ReactiveCocoa/reactivecocoa This PR adds the equivalent of RAC2's startWith operator. There's debate over whether it should be called startWith or beginsWith. Can people please weigh in?

@inamiy and @RuiAAPeres have argued that there's potential confusion between startWith and startWithNext, etc.

@mdiep has argued that there's a helpful parallel between startWith and startWithNext, that they have different types, and that the compiler will point out if you use it wrong.

@JaviSoto
Copy link
Member

My 2 cents: in my codebase I named this in an extension startWithValue(). I was also concerned with the similarity with the other startWith methods, but I thought the keyword "value" did a good job at disambiguating.

However, it might be good to consider how we're gonna name these methods with Swift 3 and the new API naming guidelines. Perhaps the one that returns a disposable could be func start(next:error:etc:). And that leaves this method (which btw I think is very useful to have) kind of in a tough spot. I think func begin(value:) would make sense.

Somewhat unrelated to this, I've also found a startWithNil() method that maps the signal into T? to be very useful to have!

@mdiep
Copy link
Contributor

mdiep commented May 30, 2016

Maybe it would work to go a different route and name it something like prefix(value:)?

@neilpa
Copy link
Member

neilpa commented May 30, 2016

Throwing out another option from left field -- prefix. IMO, both startWith and beginWith suggest that they subscribe the underlying producer at the call-site. Using begins as @mdiep suggested is better but subtle and still potentially confusing. Something like prefix is more obviously declarative.

@neilpa
Copy link
Member

neilpa commented May 30, 2016

Lol, @mdiep beat me by seconds. Good sign that we had the same suggestion independently.

@inamiy
Copy link
Contributor Author

inamiy commented May 30, 2016

prefix(value:) sounds nicer to me!
I was worried if someone says begin or beginWith does not actually begin SignalProducer 😝

If it's no problem, I will rename this method and GH-issue title.

@mdiep
Copy link
Contributor

mdiep commented May 30, 2016

Go for it, @inamiy!

@inamiy inamiy changed the title Add beginWith() Add prefix(value:) May 30, 2016
@inamiy
Copy link
Contributor Author

inamiy commented May 30, 2016

Done renaming in 608d501!

@mdiep
Copy link
Contributor

mdiep commented May 30, 2016

Thanks @inamiy! This is great. ✨

And I'm glad we settled on prefix(value:). ☺️

@mdiep mdiep merged commit e0dbf05 into ReactiveCocoa:master May 30, 2016
@inamiy
Copy link
Contributor Author

inamiy commented May 30, 2016

@mdiep Thanks for merge & good naming! ✨

@inamiy inamiy deleted the beginWith branch May 30, 2016 23:42
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

5 participants