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

Combinator to merge two futures yielding the same types #271

Merged
merged 4 commits into from Dec 8, 2016

Conversation

Projects
None yet
7 participants
@ghost
Copy link

ghost commented Dec 1, 2016

Supposed to solve #270.

I chose the name Branch to be consistent with future similar types with more variants (if ever useful), like Branch3, Branch4, etc, instead of Haskell's Either.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Dec 1, 2016

Thanks for the PR! Could this be a new module, src/future/branch.rs, with reexports in src/future/mod.rs? (Like the other combinators).

I also wonder if we should call this Branch2 to have the naming be extensible into more variants later?

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Dec 1, 2016

cc @tailhook you tend to have a lot of insights on naming these combinators and such, does this look ok?

@Stebalien

This comment has been minimized.

Copy link
Contributor

Stebalien commented Dec 1, 2016

FYI, @bluss has already created an either crate for use cases like this so you might consider simply implementing Future on Either. Multiple branches can be achieved through recursion, albeit not quite as efficiently at the moment (better layout optimizations may fix that).

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Dec 1, 2016

I'd prefer to keep the dependencies slim for now if possible, but that's certainly an option to keep open (and can be done in parallel if need be)

@carllerche

This comment has been minimized.

Copy link
Contributor

carllerche commented Dec 2, 2016

Personally I prefer the either name. I don't really think it has a hard implication of two variants.

@ghost

This comment has been minimized.

Copy link
Author

ghost commented Dec 2, 2016

I initially called it IfElse.

I don't really mind, but I've tried not to use Either because it's not very readable in Haskell, where it means both Result and "Branch combinator".

I'm just realising it's not a good reason, since Rust has an actual Result.

@spinda

This comment has been minimized.

Copy link
Contributor

spinda commented Dec 7, 2016

@pijul Do you plan on updating this so it can be merged in?

@ghost

This comment has been minimized.

Copy link
Author

ghost commented Dec 7, 2016

Update in which way?

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Dec 7, 2016

Ah yeah I'd still like to discuss naming a bit here before landing (there's quite a few possibilities). @aturon do you have thoughts on the particular names used?

@tailhook

This comment has been minimized.

Copy link
Contributor

tailhook commented Dec 7, 2016

Well, the naming part is unclear here (this is actually why I haven't submitted a similar PR a while ago :) ).

Personally, I like current version:

if something {
  Branch2::A(something)
} else {
  Branch2::B(something_else)
}

I.e. it reads like branch 1 from 2 (but backwards).

The IfElse and Either could be okay but I think more than two branches are at least as useful as join5 that we already have.

@aturon

This comment has been minimized.

Copy link
Collaborator

aturon commented Dec 8, 2016

My preference is for Either, and then later Either3, Either4 etc if needed. Mostly because of the strong precedent and straightforward reading.

Another question is what to call the variants. I've also seen these called Left and Right or First and Second, but just using single letters as this PR does seems like an improvement to me!

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Dec 8, 2016

Ok, @pijul want to rename this to Either but leave the names A and B?

@ghost

This comment has been minimized.

Copy link
Author

ghost commented Dec 8, 2016

Sure!

@alexcrichton alexcrichton merged commit b66309f into rust-lang-nursery:master Dec 8, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Dec 8, 2016

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.