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

<RACStream> monad, RACSequence #92

Merged
merged 80 commits into from Nov 20, 2012
Merged

<RACStream> monad, RACSequence #92

merged 80 commits into from Nov 20, 2012

Conversation

jspahrsummers
Copy link
Member

#89

Added a lazy RACSequence class, and abstracted common APIs between it and <RACSubscribable> into a new <RACStream> protocol. I'm definitely open to better names for these guys.

RAC should be tagged before this gets merged in.

Also fixed some header visibility attributes.
Although not necessarily part of the definition of a monad (though Haskell's MonadPlus includes it), it's useful for our stream abstraction.
This is equivalent to mplus.
Although equivalent to monadic join, and thus technically redundant in the presence of bind, it'll be extremely convenient to have. Without lazy evaluation, it's quite hard to implement a generic, cheap join.
@Coneko
Copy link
Member

Coneko commented Nov 1, 2012

I don't like methods being declared as properties when they're not KVO compliant, but I understand nonatomic and copy make for good hints on what considerations must be made when calling them and cocoa collection classes can't be observed anyway, so I can't really say declaring them as plain methods would be better in these cases.

@Coneko
Copy link
Member

Coneko commented Nov 1, 2012

-flatten would probably be better called -join, and could be implemented as

- (id<RACStream>)join {
  return [self bind:^id<RACStream>(id value) {
    return value;
  }];
}

so it doesn't have to be a required method.

@joshaber
Copy link
Member

joshaber commented Nov 1, 2012

BTW, I fully expect some do syntax using crazy macros :trollface:


// Returns all but the first `count` objects in the sequence. If `count` exceeds
// the number of items in the sequence, nil is returned.
- (RACSequence *)drop:(NSUInteger)count;
Copy link
Member

Choose a reason for hiding this comment

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

Why would this need to be on RACSequence instead of RACStream?

Copy link
Member

Choose a reason for hiding this comment

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

He did mention he still had to move some methods from RACSequence and RACSubscribable to RACStream.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, @Coneko nailed it. This was added before the protocol (just to demonstrate to myself that the primitives I had were good enough). It's going to go into <RACStream> later.

@joshaber
Copy link
Member

joshaber commented Nov 1, 2012

Overall I ❤️ ❤️ ❤️ the direction this is going 👍

#import "EXTConcreteProtocol.h"

// A concrete protocol representing any stream of values. Implemented by
// RACSubscribable and RACStream.
Copy link
Member

Choose a reason for hiding this comment

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

Pretty sure you mean RACSequence here.

Copy link
Member Author

Choose a reason for hiding this comment

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

derp

@jspahrsummers
Copy link
Member Author

@Coneko Good call on the default implementation of -flatten.

Although join is a more correct name, I'm hesitant to use terminology which is going to be extremely unfamiliar to Cocoa programmers, especially when clearer alternatives exist.

The use case is too narrow, and it's trivial to implement from scratch.
Added rac_keySequence for the old behavior.
<RACStream>'s implementation is good enough now that we have lazy -bind:.
<RACSubscribable> implicitly includes it.
Left +merge: as-is, because its naming is clearer when given an array of subscribables to combine.
@jspahrsummers
Copy link
Member Author

I still can't think of a solution to the take: problem without giving the flattenMap: block another parameter. Haskell doesn't really suffer from this problem because everything is lazy by default.

@Coneko
Copy link
Member

Coneko commented Nov 8, 2012

Why not take the easy way out and have RACSubscribable have a custom implementation of take: instead of inheriting the one from <RACStream>?

@joshaber
Copy link
Member

joshaber commented Nov 9, 2012

Yeah, though I bet -take: won't be the only case with this problem.

@jspahrsummers
Copy link
Member Author

It's worth noting that sequences have a similar, though less severe, problem. take:1 shouldn't result in the original sequence trying to resolve two values.

It'd be nice to have a generic solution for <RACStream>.

// Appends the values of `stream` to the values in the receiver.
//
// stream - A stream to concatenate. This must be an instance of the same
// concrete class as the receiver, and should not be `nil`.
Copy link
Member

Choose a reason for hiding this comment

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

You should change stream to be id instead of id<RACStream> since all other cases where a stream of the same type was required have been typed with just id.

Copy link
Member Author

Choose a reason for hiding this comment

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

Those cases (return types of blocks) have been because of Objective-C's limited type system. When we can type things as id<RACStream> without issue, we should.

@Coneko
Copy link
Member

Coneko commented Nov 9, 2012

It mimics the "terminating subscriptions immediately" issue in #94 doesn't it? In both cases the existing interface is elegant, but doesn't cover some very particular edge cases, and having to dirty the interface to cover them is frustrating.

On an unrelated note, how come sequences only have a class cluster, and not a concrete protocol too like subscribables?

@jspahrsummers
Copy link
Member Author

On an unrelated note, how come sequences only have a class cluster, and not a concrete protocol too like subscribables?

I think if you have a type that you want to be sequencable, it's trivial to write an adapter, just like what's done for the built-in collection classes right now.

Honestly, I would rather subscribables not be a protocol, but there are some legitimate use cases for it that I don't find as applicable to sequences. See #35 for some more discussion.

@jspahrsummers
Copy link
Member Author

Added -[RACSequence subscribableWithScheduler:] and <RACSubscribable>.sequence to convert between the two. Also fixed the issue with take: (for both kinds of stream) by adding a BOOL *stop parameter to the block of flattenMap:.

@Coneko
Copy link
Member

Coneko commented Nov 10, 2012

I'd say -flattenMap: doesn't need to accept a return value of nil anymore, one can return empty if she/he wants to stop without adding a value.
Users might otherwise think when you return nil setting the stop flag doesn't make any difference.

@jspahrsummers
Copy link
Member Author

There is a semantic difference between returning nil and returning empty, though. My hope is that binds won't generally need to be terminated by users anyways – the stop parameter is more for implementing other <RACStream> methods.

@joshaber
Copy link
Member

Alright so the one last thing. We talked about this in person, but I think it's better to go back to -bind: being the primitive and then provide a -flattenMap: without the stop parameter.

@jspahrsummers
Copy link
Member Author

💥

Conflicts:
	ReactiveCocoaFramework/ReactiveCocoa/RACSubscribableProtocol.h
@joshaber
Copy link
Member

✊ 👊 ✌️

joshaber added a commit that referenced this pull request Nov 20, 2012
<RACStream> monad, RACSequence
@joshaber joshaber merged commit 6372f65 into master Nov 20, 2012
andersio pushed a commit that referenced this pull request Sep 22, 2016
Disable broken UIDatePicker test
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