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

deprecate readstring to read(io, String) #22793

Closed
JeffBezanson opened this issue Jul 13, 2017 · 10 comments · Fixed by #22864
Closed

deprecate readstring to read(io, String) #22793

JeffBezanson opened this issue Jul 13, 2017 · 10 comments · Fixed by #22864
Labels
domain:io Involving the I/O subsystem: libuv, read, write, etc. domain:strings "Strings!" good first issue Indicates a good issue for first-time contributors to Julia kind:deprecation This change introduces or involves a deprecation status:help wanted Indicates that a maintainer wants help on an issue or pull request
Milestone

Comments

@JeffBezanson
Copy link
Sponsor Member

This is more consistent with other read methods, and more easily extensible to other types of strings.

@JeffBezanson JeffBezanson added kind:deprecation This change introduces or involves a deprecation domain:io Involving the I/O subsystem: libuv, read, write, etc. domain:strings "Strings!" labels Jul 13, 2017
@JeffBezanson JeffBezanson added this to the 1.0 milestone Jul 13, 2017
@JeffBezanson JeffBezanson added good first issue Indicates a good issue for first-time contributors to Julia status:help wanted Indicates that a maintainer wants help on an issue or pull request labels Jul 18, 2017
@jpfairbanks
Copy link
Contributor

jpfairbanks commented Jul 19, 2017

This sounds straightforward you just:

  • remove the definition of readstring
  • add the new method for read(s::IO, ::Type{String})
  • @deprecate in the base/deprecated.jl
  • chase down all the deprecation warnings in Base
  • Update the docs
  • Add NEWS entry

Are there any other steps?

@JeffBezanson
Copy link
Sponsor Member Author

Correct!

@jpfairbanks
Copy link
Contributor

This example in the docs doesn't work .

"""
    open(f::Function, args...)

Apply the function `f` to the result of `open(args...)` and close the resulting file
descriptor upon completion.

**Example**: `open(readstring, "file.txt")`
"""

You could make an anonymous function. open(f->read(f, String), "file.txt") but that looks weird.

@jpfairbanks
Copy link
Contributor

Also what do you do with this line in precompile.jl

precompile(Tuple{typeof(Base.open), typeof(Base.readstring), String})
?

@JeffBezanson
Copy link
Sponsor Member Author

I think open(f->read(f, String), "file.txt") is fine, or maybe open(read, "file.bin") instead.

The precompile line can just be deleted; those need to be regenerated periodically anyway.

@jpfairbanks
Copy link
Contributor

Ok, I went with f-read(f,String) and deleted it. The PR is open and CI failed due to some documentation building stuff. I don't know what to do about the CI.

JeffBezanson pushed a commit that referenced this issue Jul 20, 2017
deprecate readstring for read(io, String). fixes #22793.
jeffwong pushed a commit to jeffwong/julia that referenced this issue Jul 24, 2017
ssfrr pushed a commit to JuliaAudio/SampledSignals.jl that referenced this issue Aug 18, 2018
`readstring` seems to be deprecated and moved to `read(io, String)` in Julia 1.0, according to the following issue.

JuliaLang/julia#22793
@matklad
Copy link
Contributor

matklad commented Sep 29, 2020

Low-weight opinion of a new Julia user, but I personally would appreciate if there were Base.readstring(io) = read(io, String) alias defined.

readstring(io) is easier to type than read(io, String) and also is cognitively easier to understand -- I won't be thinking "cool, Julia can dispatch on types", the code would be just boring. This is a bit of a concrete abstraction smell, as fixing the second argument to String would be pretty common I guess.

@StefanKarpinski
Copy link
Sponsor Member

StefanKarpinski commented Sep 29, 2020

Personally, I miss readall(io) which is how this was originally spelled. Why does that spelling make sense?

  1. Whereas read(io) by itself produces bytes, readline, readlines, readuntil and readchomp all produce strings, so there is strong precedent for something called readxxx producing strings.

  2. Calling it readstring is a bit ambiguous since there are many ways to read a string from an IO stream, such as all of those functions. For example, it would also be arguable that something called readstring could read a NUL-terminated string from an IO stream. Julia doesn't use NUL-terminated strings, so I'm not arguing for that, but the point is that readstring and even read(io, String) are not that clear. Read a string how?

  3. The key behavior of readall is that it reads all of the remaining data as a string. The fact that readxxx functions generally produce strings already implies that it produces a string; calling it readall makes that as clear as possible.

Therefore, I propose that we reinstate readall(io) as an alias for read(io, String).

@matklad
Copy link
Contributor

matklad commented Sep 29, 2020

Huh, TIL about readchomp. It actually fully replaces the need for readall for me: matklad/config@6634ca5. I didn't realize that it could exists, I think advertising it in the docs might help: #37806!

@StefanKarpinski
Copy link
Sponsor Member

readchomp is one of my favorites 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:io Involving the I/O subsystem: libuv, read, write, etc. domain:strings "Strings!" good first issue Indicates a good issue for first-time contributors to Julia kind:deprecation This change introduces or involves a deprecation status:help wanted Indicates that a maintainer wants help on an issue or pull request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants