-
-
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
Deprecate readstring in favor of read(io, String) #22864
Deprecate readstring in favor of read(io, String) #22864
Conversation
8b0603a
to
0872f95
Compare
This looks like CI is failing outside of the main Base.runtests(). This PR should only affect Base so I don't know what is going on in the travis logs.
|
The text is assumed to be encoded in UTF-8. | ||
""" | ||
readstring(s::IO) = String(read(s)) | ||
readstring(filename::AbstractString) = open(readstring, filename) |
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 method is still relevant, isn't it? I didn't see it redefined elsewhere in the diff but maybe I missed it. In this case it'd just be
read(filename::AbstractString, ::Type{String}) = open(io -> read(io, String), filename)
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.
I responded to this below
base/io.jl L 167 https://github.com/JuliaLang/julia/pull/22864/files#diff-775ff60d66216b33269fc5fcb6be714fR167 read on a filename lifts that for you right? """
read(filename::AbstractString, args...)
Open a file and read its contents. `args` is passed to `read`: this is equivalent to
`open(io->read(io, args...), filename)`.
"""
read(filename::AbstractString, args...) = open(io->read(io, args...), filename) |
Oh yeah, you're right, it should indeed go through that method. |
The problem with the build had something to do with a |
There is a problem finding references to code I didn't deprecate: |
yeah that's been happening in a few places. something is buggy, not sure what. |
Works on FreeBSD! |
base/deprecated.jl
Outdated
|
||
@deprecate readstring(s::IO) read(s, String) | ||
@deprecate readstring(filename::String) read(filename, String) | ||
@deprecate readstring(cmd::AbstractCmd) read(cmd, String) |
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 nix the extra empty line above these definitions and restore the empty line before # END ...
?
base/iostream.jl
Outdated
@@ -144,7 +144,7 @@ end | |||
Apply the function `f` to the result of `open(args...)` and close the resulting file | |||
descriptor upon completion. | |||
|
|||
**Example**: `open(readstring, "file.txt")` | |||
**Example**: `open(f->read(f, String), "file.txt")` |
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.
Tangentially, perhaps change this line to match the emerging convention for examples?
# Examples
```julia-repl
... [example] ...
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.
Like this?
"""
open(f::Function, args...)
Apply the function f
to the result of open(args...)
and close the resulting file
descriptor upon completion.
Examples
open(f->read(f, String), "file.txt")
"""
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.
julia> print(longmeeting(longpr))
lgtm! :)
base/deprecated.jl
Outdated
@@ -1581,6 +1581,10 @@ end | |||
|
|||
@deprecate String(io::GenericIOBuffer) String(take!(copy(io))) | |||
|
|||
|
|||
@deprecate readstring(s::IO) read(s, String) | |||
@deprecate readstring(filename::String) read(filename, String) |
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 seems to be tighter than the removed method, should be AbstractString
?
readstring(stream::IO) | ||
readstring(filename::AbstractString) | ||
|
||
Read the entire contents of an I/O stream or a file as a string. |
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 new method's signature should be added to the read
docstring
I see one more occurrence at |
All review comments are handled and the tests pass |
Thanks for working on this, James. |
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.
Looks good to me, just two super minor things. You could put those in a separate commit with [ci skip]
in the commit message, which will keep the CI builds from running again for two tiny doc changes.
@@ -144,7 +144,10 @@ end | |||
Apply the function `f` to the result of `open(args...)` and close the resulting file | |||
descriptor upon completion. | |||
|
|||
**Example**: `open(readstring, "file.txt")` | |||
# Examples | |||
```julia-repl |
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.
julia-repl
blocks are for things that look like REPL sessions, i.e. have julia>
input lines and lines of output. Since this doesn't have a "prompt" in front of it, I think it should just be julia
instead of julia-repl
.
|
||
read(stream::IO, String) | ||
|
||
Read the entirety of `stream`, as a String. |
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 consistency with the other docstring you added, I'd remove the comma and lowercase string. If you leave it uppercase as in the type, it should have code formatting.
Whoops. Too slow. Never mind. 😛 Nice work here! |
deprecate readstring for read(io, String). fixes JuliaLang#22793.
Here is a a pull request for Deprecating
readstring
in favor ofread(io, String)
closes #22793.