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

Add fs::read_to_string #34857

Closed
brson opened this issue Jul 16, 2016 · 10 comments
Closed

Add fs::read_to_string #34857

brson opened this issue Jul 16, 2016 · 10 comments
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. P-low Low priority T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@brson
Copy link
Contributor

brson commented Jul 16, 2016

Every time I want to load the contents of a file to a String I think this same thing: "there should be a fs::read_to_string".

It looks like to me the most convenient way to do this once ? is stable (which doesn't seem to be near) might be File::open(path)?.read_to_string()?, assuming you've got the Read trait imported. It's just not as nice.

We as @rust-lang/libs can be more aggressive about identifying and adding simple ergonomic improvements like this.

@brson brson added A-libs C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jul 16, 2016
@retep998
Copy link
Member

retep998 commented Jul 16, 2016

What if we had a read_to_vec instead and then you convert that into String?

Oh also, there is already a read_to_string method that takes a &mut String, so you'll need a better name.

@Stebalien
Copy link
Contributor

Stebalien commented Jul 17, 2016

Alternatively, we could make it generic:

// Call as `fs::read::<String>("my_file")`. Alternatively, we could try landing default type arguments :)
mod fs {
    fn read<T: SomeTrait, P: AsRef<Path>>(path: P) -> T { /* ... */ }
}

SomeTrait could just be Write but that would be a bit inefficient (it would require an intermediate buffer).

@gkoz
Copy link
Contributor

gkoz commented Jul 17, 2016

An fs::read_to_string would encourage trying to slurp files regardless of their size. Going through Read you can use take (but it won't tell you if the input is too big 😞).

BTW is trying to read the whole file into memory an anti-pattern? Apparently toml-rs only parses &str but not T: Read. If it did take any reader would you still need to read_to_string so often?

@Stebalien
Copy link
Contributor

@gkoz I think this is one of those "pay for what you use" things. If you want to do things efficiently, manually read the file one chunk at a time; if you just need to read the file into memory and know it isn't malicious (e.g., a config file), you can use the helper function. However, your two caveats should definitely be mentioned in the documentation.

@brson brson added P-low Low priority and removed I-nominated labels Jul 18, 2016
@brson
Copy link
Contributor Author

brson commented Jul 18, 2016

Libs team discussed and is amenable to a convenience, though the design may need some consideration still. Some concrete points brought up:

  • Taking &mut String like Read::read_to_string probably the wrong choice for the convenience method - should return String
  • If it doesn't take &mut String then the name read_to_string may not be right because it doesn't look like Read::read_to_string.
  • Maybe it should be a method on Path.

@kvark
Copy link
Contributor

kvark commented Jul 27, 2016

Can we just impl<R: Read> Into<String> for R {...}?

@joshtriplett
Copy link
Member

@kvark That would work for files, but would conflict with instances of Into for other implementations of Read, notably the implementation for String.

@matklad
Copy link
Member

matklad commented Mar 31, 2017

There's a crate for that: https://github.com/pornel/rust-file

@binarybana
Copy link

For naming, how about read_as_str to indicate that it is allocating a new String for you?

@Mark-Simulacrum
Copy link
Member

The questions that were already brought up by the libs team and general uncertainty I can see about this lead me to believe it requires an RFC. As such, I'm closing -- if someone wants to pursue this, please follow the process outlined here: https://github.com/rust-lang/rfcs#before-creating-an-rfc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. P-low Low priority T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

10 participants