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

Adding in note about how to handle Futures / Streams #34

Merged
merged 3 commits into from
Mar 20, 2019
Merged

Conversation

Pauan
Copy link
Contributor

@Pauan Pauan commented Mar 20, 2019

See #27

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

This looks great, thanks @Pauan! Would you also like to add a note to the design section below about how we decided we want to build higher-level APIs on top of mid-level APIs, and the "onion layer approach" where we make sure every layer is reusable? Happy if that is in this PR or in a follow up, so if you want to do a follow up, feel free to just merge this PR now and then open a new one.

Thanks again!

@fitzgen
Copy link
Member

fitzgen commented Mar 20, 2019

Oh also: since Future and Stream are both defined in the futures crate, does it make sense to have separate submodules for them? Should they both be inside the crate's futures submodule?

@fitzgen
Copy link
Member

fitzgen commented Mar 20, 2019

Oh also: we should mention that the futures usage should be behind a "futures" cargo feature!

@fitzgen
Copy link
Member

fitzgen commented Mar 20, 2019

Also also: should the top-level gloo crate have a futures feature that enables all of the sub-crates' futures features?

I'm leaning towards "yes" for convenience, but I'm also interested in what others think.

@Pauan
Copy link
Contributor Author

Pauan commented Mar 20, 2019

Oh also: since Future and Stream are both defined in the futures crate, does it make sense to have separate submodules for them? Should they both be inside the crate's futures submodule?

The futures (plural) crate has separate future and stream (singular) submodules, so I just went with that.

I don't have strong opinions one way or the other, but I do like segregating by types, and it also paves the way to other types (like signal).

Also also: should the top-level gloo crate have a futures feature that enables all of the sub-crates' futures features?

That sounds good to me.

@Pauan
Copy link
Contributor Author

Pauan commented Mar 20, 2019

I added a note about high-level and mid-level APIs.

I think we need to have consensus on what the future/stream cargo feature should be called before I make a note about it.

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Thanks!

@fitzgen fitzgen merged commit 0063e92 into master Mar 20, 2019
@fitzgen fitzgen deleted the Pauan-patch-1 branch March 20, 2019 19:27
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.

2 participants