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

tokio-current-thread crate #370

Merged
merged 5 commits into from
Jun 12, 2018

Conversation

jpbriquet
Copy link
Contributor

Hello Tokio team,

Here is a pull request for the issue (#356) :
Extract tokio::executor::current_thread to a tokio-current-thread crate

I was not sure if the crate documentation should be located in the tokio executor module (like the thread-pool one), or directly in the tokio-current-thread crate.
So, I've done the same than for the threadpool, the tokio-current-thread crate documentation is only a summary, and the full detailed documentation is located in the executor module.

It is my first contribution and also my first work on a rust project, so let me know if I need to do some changes.

@carllerche
Copy link
Member

Thanks so much for taking a stab at this. Sorry for the delay, I have been away from a computer. I will be reviewing this shortly.

Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! Looks pretty good. I had some minor comments inline.

//! [`CurrentThread`]: struct.CurrentThread.html
//! [`Future::poll`]: https://docs.rs/futures/0.1/futures/future/trait.Future.html#tymethod.poll

pub use tokio_current_thread::{
Copy link
Member

Choose a reason for hiding this comment

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

It looks like there are some re-exports missing (https://docs.rs/tokio/0.1.6/tokio/executor/current_thread/index.html)

  • BlockError
  • RunTimeoutError
  • Turn
  • TurnError
  • spawn

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I've added re-exports for all missing public struct and fn which are not deprecated.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, that is a good point.

The deprecated APIs have to stay in tokio::executor::current_thread, but they don't actually have to move to the new crate. This is to maintain backwards compatibility.

Is it possible to do that? (keep them in the old location w/o introducing them in thew new ones).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right it must stay in the old location to no break code which still use the deprecated API. I will take a look on it at the end of the week, and will then update the PR.

Thanks for the review !

# - Update html_root_url.
# - Update CHANGELOG.md.
# - Create "v0.1.x" git tag.
version = "0.1.6"
Copy link
Member

Choose a reason for hiding this comment

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

This should be "0.1.0" since it is the first release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed the crate to the 0.1.0 version and I've also adjusted deprecated annotations which were pointing on the non existing 0.1.2 version.

//! [`Future::poll`]: https://docs.rs/futures/0.1/futures/future/trait.Future.html#tymethod.poll
//! [executor module]: https://docs.rs/tokio/0.1/tokio/executor/index.html

#![doc(html_root_url = "https://docs.rs/tokio-current-thread/0.1.6")]
Copy link
Member

Choose a reason for hiding this comment

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

This should also be "0.1.0"

Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

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

Looking good. I only have one more thought (left an inline comment).

//! [`CurrentThread`]: struct.CurrentThread.html
//! [`Future::poll`]: https://docs.rs/futures/0.1/futures/future/trait.Future.html#tymethod.poll

pub use tokio_current_thread::{
Copy link
Member

Choose a reason for hiding this comment

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

Actually, that is a good point.

The deprecated APIs have to stay in tokio::executor::current_thread, but they don't actually have to move to the new crate. This is to maintain backwards compatibility.

Is it possible to do that? (keep them in the old location w/o introducing them in thew new ones).

Jean-Pascal Briquet added 5 commits June 9, 2018 21:21
@jpbriquet
Copy link
Contributor Author

@carllerche As promised, I have adjusted the PR to be backward compatible. I have not found any tests related to the deprecated API, so, just to be sure, I've checked that I could call this API on my side.

The code has also been rebased, thus latest current_thread changes ("Handle" struct) is now integrated.

As always let me know if I need to change something.

Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

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

Thanks! This looks good to me.

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