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

Move tokio_io::io to tokio::io #586

Closed
wants to merge 1 commit into from
Closed

Move tokio_io::io to tokio::io #586

wants to merge 1 commit into from

Conversation

phungleson
Copy link
Contributor

Motivation

Move tokio_io::io to tokio::io as part of #354

Solution

As suggested in #354

There are a few places need some discussions, read below.

ErrorKind,
Result,
// Read,
// Write,
Copy link
Contributor Author

@phungleson phungleson Aug 27, 2018

Choose a reason for hiding this comment

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

This was commented out due to conflicts with pub use self::read::{read, Read}; above.

What should we do in this case?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what we ought to do about that one off the top of my head --- would love to get @carllerche's opinion.

Copy link
Member

Choose a reason for hiding this comment

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

The two options here are to keep the types in a sub module or to rename them...

Given that the types themselves are less useful, I would opt to keep them in a submodule. I'm not sure what to name it though.

@@ -53,7 +53,7 @@ mod framed;
mod framed_read;
mod framed_write;
mod length_delimited;
mod lines;
pub mod lines;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed as tokio::io needs to pub use it, so the temp docs in trunk below is needed.

What should we do in this case?

Copy link
Member

Choose a reason for hiding this comment

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

Since tokio-io exports lines/Lines publicly in the tokio_io::io module (although the code lives in the private tokio_io::lines module), and we're moving everything in tokio_io::io, I think we ought to copy the lines module into tokio::io as well. Then, tokio::io can just pub use self::lines::{lines, Lines}, and we don't have to make thispub.

We should be able to deprecate the lines exported in the old location after doing so.

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

You're definitely on the right track here, this looks good.

I noticed that when I checked out this branch and ran the tests, the test tests/pipe_hup.rs still uses read_to_end from the old (deprecated) location:

warning: use of deprecated item 'tokio_io::io::read_to_end': Moved to tokio::io
  --> tests/pipe-hup.rs:20:5
   |
20 | use tokio_io::io::read_to_end;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: #[warn(deprecated)] on by default

warning: use of deprecated item 'tokio_io::io::read_to_end': Moved to tokio::io
  --> tests/pipe-hup.rs:83:22
   |
83 |         let reader = read_to_end(source, Vec::new());
   |                      ^^^^^^^^^^^

Mind fixing that?

@@ -12,6 +12,8 @@ use AsyncWrite;
///
/// [`flush`]: fn.flush.html
#[derive(Debug)]
#[deprecated(since = "0.1.8", note = "Moved to tokio-codec")]
Copy link
Member

Choose a reason for hiding this comment

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

I believe this should say "Moved to tokio::io"?

@@ -22,6 +24,8 @@ pub struct Flush<A> {
/// This function will consume the object provided if an error happens, and
/// otherwise it will repeatedly call `flush` until it sees `Ok(())`, scheduling
/// a retry if `WouldBlock` is seen along the way.
#[deprecated(since = "0.1.8", note = "Moved to tokio-codec")]
Copy link
Member

Choose a reason for hiding this comment

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

Same as above --- this was moved to tokio::io, not tokio-codec.

@@ -53,7 +53,7 @@ mod framed;
mod framed_read;
mod framed_write;
mod length_delimited;
mod lines;
pub mod lines;
Copy link
Member

Choose a reason for hiding this comment

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

Since tokio-io exports lines/Lines publicly in the tokio_io::io module (although the code lives in the private tokio_io::lines module), and we're moving everything in tokio_io::io, I think we ought to copy the lines module into tokio::io as well. Then, tokio::io can just pub use self::lines::{lines, Lines}, and we don't have to make thispub.

We should be able to deprecate the lines exported in the old location after doing so.

ErrorKind,
Result,
// Read,
// Write,
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what we ought to do about that one off the top of my head --- would love to get @carllerche's opinion.

@phungleson
Copy link
Contributor Author

phungleson commented Aug 28, 2018

Hey @hawkw I made the changes, and travis shows some weird warning, do you think we should fix in a separate PR?

image

@hawkw
Copy link
Member

hawkw commented Aug 28, 2018

@phungleson those warnings should be fixed in #575.

@carllerche
Copy link
Member

So... I just realized that this is going to be a breaking change... moving the functions over means that the return types are different and anything that has depended on them in the past will break... :(

That means this change cannot be applied until 0.2, which is sad.

@carllerche
Copy link
Member

I'm going to close this PR for now simply since no further action can be taken until a breaking change happens. However, it is tracked by #591 and once the time comes, I will open it again.

Thanks for the work!

@carllerche carllerche closed this Aug 28, 2018
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.

3 participants