-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Open keywords #25684
Open keywords #25684
Conversation
This reverts commit 5752d57.
@@ -213,23 +213,23 @@ fdio(fd::Integer, own::Bool=false) = fdio(string("<fd ",fd,">"), fd, own) | |||
|
|||
|
|||
""" | |||
open(filename::AbstractString, [read::Bool, write::Bool, create::Bool, truncate::Bool, append::Bool]) -> IOStream | |||
open(filename::AbstractString; write::Bool = true, read::Bool = !write, create::Bool = true, truncate::Bool = true, append::Bool = true) -> IOStream |
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.
You might want to wrap the line here.
systemerror("seeking to end of file $fname", ccall(:ios_seek_end, Int64, (Ptr{Cvoid},), s.ios) != 0) | ||
end | ||
return s | ||
end | ||
open(fname::AbstractString) = open(fname, true, false, false, false, false) | ||
open(fname::AbstractString) = open(fname; read = true, write = false, create = false, truncate = false, append = false) |
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.
This overwrites the above method with a different set of keyword arguments.
|
||
Open a file in a mode specified by five boolean arguments. The default is to open files for | ||
reading only. Return a stream for accessing the file. | ||
""" | ||
function open(fname::AbstractString, rd::Bool, wr::Bool, cr::Bool, tr::Bool, ff::Bool) | ||
function open(fname::AbstractString; write::Bool = false, read::Bool = !write, create::Bool = false, truncate::Bool = false, append::Bool = false) |
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.
There should be some relationship between truncate
and append
. Probably truncate = !append
. Otherwise the behavior of open(file, append=true)
is going to be rather surprising. I also suspect that write = append
makes sense. Similarly, create
typically default to true if write
is set. I think you'll need to draw a graph of the relationships here to figure this one out – this is why this change hasn't been done yet.
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.
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.
Actually, that's not quite right since you want truncate = write && !append
.
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.
ok so
open(fname::AbstractString;
append::Bool = false,
write::Bool = append,
read::Bool = !write,
create::Bool = write,
truncate::Bool = write && !append,
)
?
Actually, I think we may well have a bunch of these implications a bit backwards. I'm going to see if I can come up with a more sensible way to define the defaults based on what they mean. |
This is way trickier than I imagined at first if you want it to have intuitive "do what I mean" behavior. I took the table of
There are two basic rules I extracted from the above table:
Unfortunately, this means that [This would be a lot simpler if we were willing to spell Put together, this leads to the following implementation for getting flag values from keywords: function open_keywords(;
read :: Union{Bool,Nothing} = nothing,
write :: Union{Bool,Nothing} = nothing,
create :: Union{Bool,Nothing} = nothing,
truncate :: Union{Bool,Nothing} = nothing,
append :: Union{Bool,Nothing} = nothing,
)
if write === true && read !== true && append !== true
create === nothing && (create = true)
truncate === nothing && (truncate = true)
end
if truncate === true || append === true
write === nothing && (write = true)
create === nothing && (create = true)
end
write === nothing && (write = false)
read === nothing && (read = !write)
create === nothing && (create = false)
truncate === nothing && (truncate = false)
append === nothing && (append = false)
flags = Char[]
read && push!(flags, 'r')
write && push!(flags, 'w')
create && push!(flags, 'c')
truncate && push!(flags, 't')
append && push!(flags, 'a')
return String(flags)
end This has the desired properties, but I have to confess that it's a bit more complex than I expected. Of course, when in doubt, just spell out flags – they will always be respected if given explicitly. I think the only other way to go is probably to just pick simple boolean defaults and say that people need to supply any flag that they don't want to assume its default value. Assuming
This is pretty verbose, but it does have the virtue of being straightforward. Of course, all of these spellings also work as desired with the DWIM |
My gut feeling is that we should just stick to the boolean keywords but I also don't understand well enough to know. |
It may be that this interface just has too many degrees of freedom --- there are many combinations like |
They're boolean keywords no matter what. The question is about defaults. Do we require people to supply any non-default boolean values for the open(file, read=false, write=true, create=true, truncate=true) Or, do we make the defaults smarter (but more complicated) and allow them to just write this: open(file, write=true) The latter could potentially replace |
Some of these do make some sense, although they're not very common. |
What about struct FileMode
read::Bool
write::Bool
create::Bool
truncate::Bool
append::Bool
end
FileMode(; read = true, write = false, create = false, truncate = false, append = false) =
FileMode(read, write, create, truncate, append)
const read = FileMode(true, false, false, false, false)
const read_plus = FileMode(true, true, false, false, false)
const write = FileMode(false, true, true, true, false)
const write_plus = FileMode(true, true, true, true, false)
const append = FileMode(false, true, true, false, true)
const append_plus = FileMode(false, true, true, false, true) |
How is that better than |
Oh maybe it isn't. Just throwing out ideas |
Worth a shot. I think we have a few options here:
|
In that case I vote for (2). Munging a few bools and nothings is no big deal, and it can actually be a bit simpler than what you have above since AFAIK we don't actually need to form that string. |
Yes, that was just for testing and playing with it. I'll put up an alternate PR. |
replaced by #25696 |
No description provided.