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 file-open-with RFC #2615

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
8 participants
@Timmmm
Copy link

Timmmm commented Dec 16, 2018

Rendered

This RFC proposes making File::open() consistent by deprecating OpenOptions::new().read(true).write(true).open("existing_file") and adding File::open_with("existing_file", OpenOptions::new().read().write()) instead.

Suggestion based on rust-lang/rust#55762.

@Centril Centril added the T-libs label Dec 16, 2018


let mut file = File::open_with("foo.txt", OpenOptions::new().read().write());

This matches the normal way of doing "call a method with some options" in Rust, for example `TcpStream::connect(addr)` and `TcpStream::connect_timeout(addr, timeout)`.

This comment has been minimized.

@jonas-schievink

jonas-schievink Dec 16, 2018

Contributor

The current pattern, however, matches the well-known and frequently used builder pattern. Since you have to use OpenOptions anyways, I don't really see how this improves on just calling .open on that.

This comment has been minimized.

@Timmmm

Timmmm Dec 16, 2018

The improvement is discoverability and consistency. There's already File::open() and File::create() which leads users to believe that to open files you use a method on File.

If OpenOptions had been named FileOpenBuilder or something then I agree it would have been a bit clearer but given that it isn't, and that the most obvious methods to open a file are on File it seems silly that some random subset of file open methods are on OpenOptions.

This may be one of those things that feels obvious once you have learnt it, but as a Rust beginner, trust me this was weird and confusing. (And see the Stackoverflow questions for further evidence.)

This comment has been minimized.

@jonas-schievink

jonas-schievink Dec 16, 2018

Contributor

Perhaps then the docs on File::{open, create} should be improved to mention that you need to use OpenOptions to configure permissions

This comment has been minimized.

@Timmmm

Timmmm Dec 16, 2018

That would certainly be a good thing to do, however it would be better if the API were intuitive in the first place. Nobody likes reading documentation.

This comment has been minimized.

@aloucks

aloucks Dec 16, 2018

The discoverability of OpenOptions definitely has room for improvement. I recall being a little confused when I first needed it and saw nothing obvious associated with File.

As an alternative, we could add an open_options constructor to make it easier to find while staying true to the builder pattern.

From your example:

let mut file = OpenOptions::new()
    .read(true)
    .write(true)
    .open("foo.txt");

Would also be available via:

let mut file = File::open_options()
    .read(true)
    .write(true)
    .open("foo.txt");

And implemented:

impl File {
    pub fn open_options() -> OpenOptions {
        OptionOptions::new()
    }
}

This comment has been minimized.

@Ixrec

Ixrec Dec 16, 2018

Contributor

For the specific use case of opening a file, I don't think the builder pattern makes much intuitive sense. After all, you're not building a file. You're building an OpenOptions object. Or perhaps you could say you're building a file descriptor. But it seems weird to say that the file itself has been "built" before we write any content into it. Maybe it's possible to design a useful FileBuilder API where .build() actually puts content in the file, but that'd be a layer above what std is trying to provide.

I think it's clear that File::open(name, options) is a much more intuitive API. To me the only question is whether adding it and deprecating the already stable alternative is a net win for the language and its ecosystem, which is where I'll have to defer to others who know the Rust ecosystem much better.


```
impl File {
pub fn open_with(filename: &str, options: &OpenOptions) -> File {

This comment has been minimized.

@dtolnay

dtolnay Dec 16, 2018

Member

This will need to take the same AsRef<Path> type parameter as File::open.
https://doc.rust-lang.org/std/fs/struct.File.html#method.open

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Dec 16, 2018

I like @aloucks's alternative better - it solves the discoverability issue, and improves on both open_with and the status quo by not requiring you to import OpenOptions at all.

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Dec 16, 2018

RFCs aren't normally required for small API additions like this, BTW. You can just make a PR.

@Centril Centril added the A-file label Dec 17, 2018

@Timmmm

This comment has been minimized.

Copy link

Timmmm commented Dec 17, 2018

Ah ok. I think @aloucks's suggestion is an improvement too. I'll make a PR for it since it seems to have the most support.

@aldanor

This comment has been minimized.

Copy link

aldanor commented Dec 19, 2018

As an alternative, we could add an open_options constructor to make it easier to find while staying true to the builder pattern.

// bikeshedding

Another option would be to call the constructor method with_options() (instead of open_options()), like:

impl File {
    pub fn with_options() -> OpenOptions {
        Default::default()
    }
}

...

File::with_options().write(true).open("foo.txt");

The tiny difference being -- that it reads much more naturally like a proper English sentence. As in, "With options this, this, this, and that, open foo.txt".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment