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

Tracking issue for std::fs::read, read_string, and write #46588

Closed
SimonSapin opened this Issue Dec 8, 2017 · 32 comments

Comments

Projects
None yet
@SimonSapin
Copy link
Contributor

SimonSapin commented Dec 8, 2017

Implemented in #45837

New APIs in std::fs:

pub fn read<P: AsRef<Path>>(path: P) -> io::Result<Vec<u8>> { … }
pub fn read_string<P: AsRef<Path>>(path: P) -> io::Result<String> { … }
pub fn write<P: AsRef<Path>, C: AsRef<[u8]>>(path: P, contents: C) -> io::Result<()> { ... }

(read_string is based on read_to_string and so returns an error on non-UTF-8 content.)

Before:

use std::fs::File;
use std::io::Read;

let mut bytes = Vec::new();
File::open(filename)?.read_to_end(&mut bytes)?;
do_something_with(bytes)

After:

use std::fs;

do_something_with(fs::read(filename)?)
@JordiPolo

This comment has been minimized.

Copy link
Contributor

JordiPolo commented Dec 14, 2017

Implemented, so shouldn't this be closed?

@steveklabnik

This comment has been minimized.

Copy link
Member

steveklabnik commented Dec 14, 2017

@JordiPolo this issue is tracking their stabilization, not their implementation.

@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

SimonSapin commented Dec 14, 2017

By the way, is there a delay before we should start FCP for stabilization? This is niche enough that I don’t expect to get more feedback just by waiting. I’m not planning to use these functions myself until they’re stable.

@leonardo-m

This comment has been minimized.

Copy link

leonardo-m commented Dec 23, 2017

Is also adding a lazy iterator on lines a good idea?

@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

SimonSapin commented Dec 23, 2017

@leonardo-m https://doc.rust-lang.org/std/io/trait.BufRead.html#method.lines already exists. Or do you mean adding a convenience function that calls it from a filename? I personally feel less need for that function, but feel free to open a new PR for it.

@leonardo-m

This comment has been minimized.

Copy link

leonardo-m commented Dec 23, 2017

I meant a convenience function. It's often useful for small script-like programs, tests, benchmarks, online code competitions and games, etc.

@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

SimonSapin commented Jan 30, 2018

@aturon Let’s stabilize?

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Jan 30, 2018

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Jan 30, 2018

Team member @sfackler has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 30, 2018

This landed less than a month ago, right? Is that a bit rapid for stabilization?

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Jan 31, 2018

Why read_string instead of read_to_string? Seems incorrect to me.

@dtolnay

This comment has been minimized.

Copy link
Member

dtolnay commented Jan 31, 2018

I would expect something named read_to_string to take a &mut String argument, like in io::Read.

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Jan 31, 2018

@alexcrichton assuming we don't land a PR for this cycle, it's another 3 months away from hitting stable.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 31, 2018

@aturon that's true yeah, if we land on the next nightly (which branches Feb 15) I think that's an ok length of time.

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Jan 31, 2018

@dtolnay That's interesting. I don't think that is connected to to because read also takes an argument to read into.

To me, read_string seems either ungrammatical or wrong (you're not reading a String, you're reading something and constructing a String of it). I'd prefer some preposition connecting read and string, but I'm not picky - read_to_string, read_as_string, read_into_string, etc.

@mbrubeck

This comment has been minimized.

Copy link
Contributor

mbrubeck commented Feb 2, 2018

Previous discussion on the naming of read_string (which was originally read_utf8): #45837 (comment)

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Feb 16, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@daboross

This comment has been minimized.

Copy link
Contributor

daboross commented Feb 18, 2018

It seems like there was a lot of discussion about read_string vs read_utf8, but not much about the suitability of read_string vs. read_into_string. I'd agree with @withoutboats that read_string isn't right: it sounds like I'm reading something from a string, not creating a new one.

read_to_string isn't exactly right either though, it is really reading into a new string not to an existing one.

Is there any chance of changing the function to read_into_string before the FCP is over?

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Feb 18, 2018

@rfcbot concern read_string

Registering that I meant to be blocking on the name read_string.

I'd really like this to be stabilized. I think read_to_string is the best name, but I'd settle for read_into_string. I don't think read_to_string implies that its reading into an existing string - if it did, wouldn't read be called read_to_slice and read_to_end be called read_to_vec?

@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

SimonSapin commented Feb 18, 2018

I don’t have a strong opinion for the name of this function, but the Read trait already has a read_to_string method that takes &mut String.

@daboross

This comment has been minimized.

Copy link
Contributor

daboross commented Feb 18, 2018

if it did, wouldn't read be called read_to_slice and read_to_end be called read_to_vec?

I think if they were differentiating themselves from some other "default" read mode, then those would be reasonable names. Since we default to bytes, I'd say they don't need to differentiate?

I'm not sure I think read_into_string is better anymore though. I mean a distinction from Read::read_to_string would be good given that one takes a string to write to, but into and to have very similar connotations, and neither is really associated with "reading to an existing variable" more than the other.

👍 for either read_into_string or read_to_string

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Feb 26, 2018

The final comment period is now complete.

@mbrubeck

This comment has been minimized.

Copy link
Contributor

mbrubeck commented Mar 8, 2018

I would be happy with read_to_string, or maybe read_into_string. It doesn't seem like a perfect name to address all concerns is likely to come up, so it would be great to settle on a maybe-imperfect one.

The one issue with read_to_string is that our other read_to_string appends to an existing String, while this one returns a String. @dtolnay (or others), do you feel this difference is important enough that it needs a different name?

@angusholder

This comment has been minimized.

Copy link

angusholder commented Mar 15, 2018

How about read_as_string? It definitely doesn't imply a String is given to read into. According to the naming conventions, as_ is meant to be a costless conversion of borrowed -> borrowed, but I think in the context of reading from the file system the cost of verifying UTF8 can be considered to be quite minimal, and so as_ is a reasonable choice?

@daboross

This comment has been minimized.

Copy link
Contributor

daboross commented Mar 15, 2018

In fs::read_as_string, the as_string part is definitely a "minimal-cost conversion" if compared to fs::read.

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Mar 17, 2018

It seems like the naming for the string-returning function isn't super resolved, so how about stabilizing just read and write in the meantime?

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Mar 17, 2018

@sfackler sounds good to me :)

@angusholder

This comment has been minimized.

Copy link

angusholder commented Mar 17, 2018

read_as_string seems to me like it satisfies all the concerns voiced, could we just take a quick tally on that? When's the next deadline for getting into stabilisation?

@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

SimonSapin commented Mar 17, 2018

@angusholder There is no deadline for stabilizations. The only thing time-based things are release trains, https://forge.rust-lang.org/#release_info shows some dates. When a version reaches Stable, the next one graduates from Nightly to Beta at the same time and then reaches Stable 6 weeks later.

At this point I have no opinion about naming for this function. I’ll let @withoutboats pick one and resolve their concern with rfcbot.

@JustAPerson

This comment has been minimized.

Copy link
Contributor

JustAPerson commented Mar 23, 2018

looking forward to this being stabilized :) read_as_string seems good

bors added a commit that referenced this issue Mar 29, 2018

Auto merge of #49422 - mbrubeck:fs_read, r=TimNN
Stabilize fs::read and fs::write

As discussed in #46588 (comment)

bors added a commit that referenced this issue Mar 30, 2018

Auto merge of #49422 - mbrubeck:fs_read, r=TimNN
Stabilize fs::read and fs::write

As discussed in #46588 (comment)
@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

SimonSapin commented Mar 30, 2018

The libs team discussed this today and the consensus is to stabilize with the read_string method renamed to read_to_string. A stabilization PR is welcome.

@mbrubeck

This comment has been minimized.

Copy link
Contributor

mbrubeck commented Mar 30, 2018

#49422 stabilized read and write.

#49522 submitted to rename and stabilize read_to_string.

bors added a commit that referenced this issue Mar 31, 2018

Auto merge of #49522 - mbrubeck:fs_read, r=SimonSapin
Rename fs::read_string to read_to_string and stabilize

As approved in #46588 (comment)

Closes #46588.

bors added a commit that referenced this issue Apr 1, 2018

Auto merge of #49522 - mbrubeck:fs_read, r=SimonSapin
Rename fs::read_string to read_to_string and stabilize

As approved in #46588 (comment)

Closes #46588.

@bors bors closed this in #49522 Apr 1, 2018

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