-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Add file-open-with RFC #2615
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
Closed
Closed
Add file-open-with RFC #2615
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,116 @@ | ||
- Feature Name: file_open_with | ||
- Start Date: 2018-11-08 | ||
- RFC PR: | ||
- Rust Issue: #55762 | ||
|
||
# Summary | ||
[summary]: #summary | ||
|
||
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. | ||
|
||
# Motivation | ||
[motivation]: #motivation | ||
|
||
The current way to open an existing file in read-only mode is this: | ||
|
||
File::open("foo.txt") | ||
|
||
And to create a new file it is: | ||
|
||
File::create("foo.txt") | ||
|
||
But if you want to open an existing file in read/write mode it is this: | ||
|
||
use std::fs::OpenOptions; | ||
|
||
let mut file = OpenOptions::new() | ||
.read(true) | ||
.write(true) | ||
.open("foo.txt"); | ||
|
||
This is inconsistent and unexpected. I propose that we deprecate `OpenOptions::open()` and add `File::open_with(path: &str, options: &OpenOptions)`. | ||
|
||
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)`. | ||
|
||
# Guide-level explanation | ||
[guide-level-explanation]: #guide-level-explanation | ||
|
||
## Reading and Writing Files | ||
|
||
To open a file in read-only mode you can use `File::open()` like this: | ||
|
||
let mut file = File::open("existing_file"); | ||
|
||
This will open the file in read-only mode. To create a new file and write to it you can use: | ||
|
||
let mut file = File::create("new_file"); | ||
|
||
This will open the file in read/write mode and create it if it doesn't already exist. If you want to open an existing file, but *not* create it if it already exists, use this code: | ||
|
||
let mut file = File::open_with("existing_file", OpenOptions::new().read().write()); | ||
|
||
# Reference-level explanation | ||
[reference-level-explanation]: #reference-level-explanation | ||
|
||
Internally this could be implemented exactly like this: | ||
|
||
``` | ||
impl File { | ||
pub fn open_with(filename: &str, options: &OpenOptions) -> File { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will need to take the same |
||
options.open(filename) | ||
} | ||
} | ||
``` | ||
|
||
# Drawbacks | ||
[drawbacks]: #drawbacks | ||
|
||
There would now be two ways to open a read/write file (though one would be deprecated). | ||
|
||
# Rationale and alternatives | ||
[rationale-and-alternatives]: #rationale-and-alternatives | ||
|
||
This is confusing, as evidenced by [these](https://stackoverflow.com/questions/50039341/open-file-in-read-write-mode-in-rust) [two](https://stackoverflow.com/questions/47956653/is-it-possible-to-use-the-same-file-for-reading-and-writing) Stackoverflow questions. Passing an "options" struct as a parameter to a `new_with()` style constructor is the standard idiom in Rust - as seen in `TcpStream::connect_timeout()` and `SipHasher::new_with_keys()`. | ||
|
||
# Prior art | ||
[prior-art]: #prior-art | ||
|
||
The C API uses `fopen()` which has a `mode` string for this purpose. It doesn't have a mode structure that contains an `open()` function. | ||
|
||
C++ has a similar mechanism - the `fstream` constructor can take `ios::in | ios::out`. | ||
|
||
Even Haskell looks normal in comparison: | ||
|
||
handle <- openFile "file.txt" ReadWriteMode | ||
|
||
I don't know of another language that has something like Rust's | ||
|
||
file_open("read_only") | ||
read_write_options.open("read_write") | ||
|
||
# Unresolved questions | ||
[unresolved-questions]: #unresolved-questions | ||
|
||
The name. | ||
|
||
* `open_with` | ||
* `open_with_options` | ||
* Something else? | ||
|
||
# Future possibilities | ||
[future-possibilities]: #future-possibilities | ||
|
||
This is still fairly verbose: | ||
|
||
let mut file = File::open_with("existing_file", OpenOptions::new().read().write()); | ||
|
||
It may be nice to add shortcuts for common options, or to use a bitfield type thing instead: | ||
|
||
let mut file = File::open_read_write("existing_file"); | ||
let mut file = File::open_with_mode("existing_file", File::Read | File::Write); | ||
|
||
Or | ||
|
||
let mut file = File::open_with_options("existing_file", OpenOptions::read_write()); |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The improvement is discoverability and consistency. There's already
File::open()
andFile::create()
which leads users to believe that to open files you use a method onFile
.If
OpenOptions
had been namedFileOpenBuilder
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 onFile
it seems silly that some random subset of file open methods are onOpenOptions
.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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps then the docs on
File::{open, create}
should be improved to mention that you need to useOpenOptions
to configure permissionsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 withFile
.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:
Would also be available via:
And implemented:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 usefulFileBuilder
API where.build()
actually puts content in the file, but that'd be a layer above whatstd
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.