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

Migrate to futures 0.3 from 0.1 #41

Merged
merged 4 commits into from May 30, 2019

Conversation

@nrc
Copy link
Contributor

commented May 6, 2019

And some minor refactoring. Based on #40, it is worth us reviewing and landing that first.

PTAL @sunxiaoguang

@nrc nrc force-pushed the nrc:futures branch 3 times, most recently from 3c1d1dc to 43ec8a3 May 6, 2019

@nrc nrc referenced this pull request May 6, 2019

Merged

Migrate to Prost #42

@nrc nrc force-pushed the nrc:futures branch 4 times, most recently from 1a7e6cc to 4f3c6ae May 6, 2019

@nrc nrc force-pushed the nrc:futures branch from 4f3c6ae to 1f74619 May 10, 2019

@nrc

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

Rebased to remove conflict, merged the last three commits to make it easier to review

@nrc nrc force-pushed the nrc:futures branch from b5ccb58 to 8941533 May 10, 2019

@nrc nrc force-pushed the nrc:futures branch from 8941533 to 830fffe May 19, 2019

@nrc

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2019

Rebased on to #42 (since I expect that to land first). PTAL at the second and third commits @Hoverbear and @sunxiaoguang

@nrc nrc requested review from sunxiaoguang and Hoverbear May 19, 2019

@nrc nrc force-pushed the nrc:futures branch from 830fffe to 16377e3 May 20, 2019

@Hoverbear

This comment has been minimized.

Copy link
Member

commented May 20, 2019

This does cause us not to follow https://github.com/tikv/rfcs/pull/7/files#diff-3333e76d7edf3ed642c27ac78f1795a6R25 until these features are stabilized.

That said: The RFC isn't accepted.

I think it's fine. What do you think @sunxiaoguang?

@ice1000

This comment has been minimized.

Copy link
Member

commented May 20, 2019

async/await! Cool!

Show resolved Hide resolved README.md Outdated
Show resolved Hide resolved README.md Outdated
src/lib.rs Outdated
@@ -31,7 +31,7 @@
//! [dependencies]

This comment has been minimized.

Copy link
@Hoverbear

Hoverbear May 21, 2019

Member

A couple lines above this we say we support stable but we don't now!

This comment has been minimized.

Copy link
@nrc

nrc May 22, 2019

Author Contributor

Hmm, looks like we duplicate the README in the crate docs. IMO, it is better not to include installation instructions in the crates docs, just how to use the API. I've updated the PR with that change, what do you think?

@nrc nrc force-pushed the nrc:futures branch 2 times, most recently from adc5715 to 09cea49 May 22, 2019

@brson

brson approved these changes May 28, 2019

Copy link
Collaborator

left a comment

I am approving this because you need to get it landed, but it's hard to review. I don't understand either futures 0.1 or 0.3 well enough, nor do I know this code base.

I read through it all. The mechanical stuff seems correct, but there's so much that it's hard to pay attention to every line.

Obviously the compat module is the most important bit here. The SendAllCompat type is tough. I ready through all the docs of all the methods it calls, and I guess it's ok.

The only use of it though is followed by a TODO that says "I think that is ok". I can not determine this any better than you can.

nrc added some commits May 2, 2019

Migrate to futures 0.3
Signed-off-by: Nick Cameron <nrc@ncameron.org>
Use .await syntax
Signed-off-by: Nick Cameron <nrc@ncameron.org>
Update the README with info about Rust toolchains
Signed-off-by: Nick Cameron <nrc@ncameron.org>

@nrc nrc force-pushed the nrc:futures branch 3 times, most recently from 1909812 to 32a03b8 May 29, 2019

Travis CI: lock nightly to a version with Clippy
Signed-off-by: Nick Cameron <nrc@ncameron.org>

@nrc nrc force-pushed the nrc:futures branch from 32a03b8 to 719b32e May 30, 2019

@nrc nrc merged commit ddb4e42 into tikv:master May 30, 2019

2 checks passed

DCO All commits are signed off!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

nrc added a commit to nrc/client-rust that referenced this pull request Jun 4, 2019

Fix a warning
Caused by an incorrect conversion in tikv#41

Signed-off-by: Nick Cameron <nrc@ncameron.org>

@nrc nrc referenced this pull request Jun 4, 2019

Merged

Fix a warning #62

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