Skip to content

rename Read to ReadOnce and expose it #55

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

Merged
merged 2 commits into from
Oct 16, 2016

Conversation

oconnor663
Copy link
Contributor

The other read futures (read_exact, read_until, etc.) all expose their
concrete future types so that function signatures can return them, but
until now read() didn't. Exposing it with the name "Read" causes
naming conflicts with the std::io::Read trait, so the easiest thing to
do is to just change the name. Importing std::io::Read under a different
name would've been an option too, but that would probably be annoying
for consumers in the same way it's annoying for us.

The original PR (#29) decided
that "read" was a better name than "read_some", so I'm leaving the top
level functions unchanged. I don't have a strong opinion about it one
way or the other, but I do think it's worth bikeshedding a little bit.
Python's asyncio library actually ended up with a very similar issue
around naming inconsistency between the sync and async worlds, and we
can hopefully avoid repeating that: https://bugs.python.org/issue22279

The other read futures (read_exact, read_until, etc.) all expose their
concrete future types so that function signatures can return them, but
until now `read()` didn't. Exposing it with the name "Read" causes
naming conflicts with the std::io::Read trait, so the easiest thing to
do is to just change the name. Importing std::io::Read under a different
name would've been an option too, but that would probably be annoying
for consumers in the same way it's annoying for us.

The original PR (tokio-rs#29) decided
that "read" was a better name than "read_some", so I'm leaving the top
level functions unchanged. I don't have a strong opinion about it one
way or the other, but I *do* think it's worth bikeshedding a little bit.
Python's asyncio library actually ended up with a very similar issue
around naming inconsistency between the sync and async worlds, and we
can hopefully avoid repeating that: https://bugs.python.org/issue22279
@oconnor663
Copy link
Contributor Author

oconnor663 commented Oct 10, 2016

Actually, broader question: Is it weird that a function can return a struct whose type is not actually public? The current Read struct is pub in the read module, but the read module is not pub within io. Last I can find on this is from two years ago: rust-lang/rust#10573

@alexcrichton
Copy link
Contributor

Oh oops it was definitely intended for this to get reexported. The intention here was also that the type mirrors the name of the function that creates it. I was wondering if this would crop up much but I figured it wouldn't be hit that often, but did you run into this as well?

@oconnor663
Copy link
Contributor Author

Not in my own code, just in my first attempt to export it :) I guess the solution there would be to have lib.rs stop importing std::io::Read, and just call it's methods fully qualified when it needs them?

@ghost
Copy link

ghost commented Oct 15, 2016

+1 for remaining with Read as a name for consistency reasons.

The name conflict doesn't seem to be that problematic from end-user perspective
as they would be free to refer to tokio Read struct using qualified name.

Neither it is problematic internally, as io mod.rs doesn't actually call any
std::io::Read methods.

@oconnor663
Copy link
Contributor Author

The latest commit renames ReadOnce back to Read. The overall changes are now pretty small. We have to use io::Read and io::Write in a qualified way inside mod.rs to avoid the name conflict. Like you both mentioned, end users will probably do the opposite, using tokio_core::Read in a qualified way if they ever need to.

@alexcrichton
Copy link
Contributor

Ok, this seems good to stick with for now, thanks!

@alexcrichton alexcrichton merged commit 3ced812 into tokio-rs:master Oct 16, 2016
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