add back bufferless read_to_string/read_to_end methods

[cburgdorf]

The Read trait lost it's read_to_string and read_to_end methods that didn't require to pass a buffer. That comes as a small convenience loss. @steveklabnik suggested to write a PR to bring them back.

Rendered view

[huonw]

Is there a missing example before this?

[cburgdorf]

Actually it's just a superfluous Or this: ;) I updated the PR

[seanmonstar]

Don't these conflict with the existing read_to_end and read_to_string methods?

[cburgdorf]

Good point. What's the state of method overloading in Rust? I've been basically away from Rust for the last couple of months which means I have to start over. If there's no such thing as method overloading I'd need to give them other names.

[seanmonstar]

You can provide multiple methods of the same name from differing traits, and use UFCS (ie ReadExt::read_to_end(&mut rdr, Vec::new())) to differentiate. But that would not improve the convenience you mentioned in this RFC. You'd need a new name.

[reem]

read_into_vec and read_into_string, possibly

[cburgdorf]

@reem I like those, updated the PR

[netvl]

👍. There should be a simple way to do simple things.

[cburgdorf]

Also created a PR against the source rust-lang/rust#23335

[alexcrichton]

There were two concerns which motivated moving to taking a buffer instead of returning it, the first of which you've discussed here (performance), but perhaps the more important one is the loss of information. The APIs for read_into_{vec,string} can read data but then discard it if an error occurs. This form of "lossy API" we've tried to avoid having show up as much as possible, so you may want to also discuss this factor in the Motivation section to indicate why these functions are worth it.

[cburgdorf]

Sorry @alexcrichton I may not see the forrest for the trees.

How is this read_into_string implementation handling the error situation differently than the read_to_string API which it internally uses?

    fn read_into_string(&mut self, buf: &mut String) -> Result<String> {
        let mut buf = String::new();
        let res = self.read_to_string(&mut buf);
        res.map(|_| buf)
    }

[seanmonstar]

With read_to_string, even if there's an error, all previously read data has
still been pushed onto the String. With the proposed read_into_string, the
String is lost if an error occurs, even if there was success before the
error. That data is likely lost to the void.

On Sun, Mar 15, 2015, 3:13 PM Christoph Burgdorf [email protected]
wrote:

Sorry @alexcrichton https://github.com/alexcrichton I may not see the
forrest for the trees.

How is this read_into_string implementation handling the error situation
differently than the read_to_string API which it internally uses?

fn read_into_string(&mut self, buf: &mut String) -> Result<String> {
    let mut buf = String::new();
    let res = self.read_to_string(&mut buf);
    res.map(|_| buf)
}

—
Reply to this email directly or view it on GitHub
#970 (comment).

[cburgdorf]

Ah, sure. I see it. How about a special error type that will include the String. Then even if there was an error one could get the bits that were read successfully up to that point.

[alexcrichton]

@cburgdorf

Ah, sure. I see it. How about a special error type that will include the String. Then even if there was an error one could get the bits that were read successfully up to that point.

It is plausible to create a ReadIntoStringError which has a FromError implementation translating it to io::Error, but this is a lot of extra API surface for what doesn't seem like much gain. Almost all one-liners previously earn at most one extra line with a take-the-mutable-buffer approach and it allows us to knock out even more use cases in one stroke.

[cburgdorf]

but this is a lot of extra API surface for what doesn't seem like much gain

I understand the stance you take on that. We probably look at this from too different perspectives.

For me, it just feels strange not to have such methods on the Read trait. People coming from .NET, Java, Node.js or Go may be quite surprised not to have this.

The way I see it, we should follow the principle of least astonishment here.

[rkjnsn]

I like this. Getting the contents as a newly-created String or Vec is a common enough operation that I think there should be a convenient way to do it. I also like that makes chaining easier.

The APIs for read_into_{vec,string} can read data but then discard it if an error occurs.

I don't think this should be a concern. Even with the current read_to_* methods, I'm going to be using try! most of the time, so my stack-allocated String or Vec is going to be immediately discarded, anyway. Any function that needs the partially-read data can continue to use the existing methods.

I definitely don't think it's worth adding a ReadIntoStringError, as it makes the interface more complicated for very little gain.

I think combined with read_all (#980), and maybe a write_str (write_all for string slices), this would yield a nicely flushed out pair of Read and Write traits.

[reem]

I think we should wait to see how much of a problem this is once we have a larger body of rust code and add this later. This is a very easy API to add later but quite an awkward one to keep around if we decide we don't want any data-losing APIs .

[Diggsey]

Instead of specifically a ReadIntoStringError, there could be a PartialReadError or similar, which can be used by any read operation which might fail partway through. The error would allow access to the partially read data.

[cburgdorf]

Ok, for now I created a read-ext that aims to bring those APIs as extension methods. It's available on crates.io.

I didn't manage to get the tests working yet, so I'm not actually sure if it works.

[cburgdorf]

Never mind, it's working now with version 0.1.0

[archer884]

Commenting just because I'm an example of one of those .NET guys who was surprised (astonished! :P) to find there was no trivial way to do this. I guessed why it wasn't there (the reasons mentioned above) and figured the justification was that it's so easy to add that, if I want it, I'm expected to do it myself... But I'd be totally cool with this being a thing I didn't have to implement, too. :)

[cburgdorf]

because I'm an example of one of those .NET guys

Haha, awesome. You are validating my theory ;)

I'd be totally cool with this being a thing I didn't have to implement

For now just use my readext crate

[jnicklas]

Imagine the case of reading a configuration file. This will be done once at the start of the program, so is hardly performance critical, and there isn't much use in loading half a configuration file, so it's really all or nothing. That kind of thing is a lot less convenient with the revised IO APIs than it was before.

[aturon]

Thanks for the RFC!

The goal of the recent IO reform was to provide high-performance, low-level APIs that map as directly to system APIs as possible, and can be used to build various higher-level abstractions and conveniences.

While additional conveniences as proposed here may eventually be prudent to add to std, we would prefer to give the new IO system some time to bake, so that we can gain broader experience with the pain points. (We've been working hard to pare down the commitments in std to those we feel very confident about.) So for the time being, having this kind of functionality live in the crates.io ecosystem seems like the right step.