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.

Sign up for GitHub

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

Jump to bottom

Add readeach function for iterating streams #36150

Merged
merged 9 commits into from
Jul 23, 2020
Merged

Add readeach function for iterating streams #36150

merged 9 commits into from
Jul 23, 2020

Conversation

non-Jedi
Copy link
Contributor

@non-Jedi non-Jedi commented Jun 4, 2020 •
edited
Loading

See the commit messages and the discussion in #36132 for justification/explanation. I can break this up into 2 PRs if there's only consensus on merging half of this, but it's semi-related, so I'm submitting as a single PR first.

@JeffBezanson JeffBezanson added the triage This should be discussed on a triage call label Jun 4, 2020
@JeffBezanson
Copy link
Member

I think these are both basically good ideas, but I agree they should be 2 PRs.

Unfortunately with the name skipuntil the meaning of the predicate should be inverted, so it's not a drop-in replacement for skipchars (but it could still replace it eventually).

@non-Jedi
Copy link
Contributor Author

non-Jedi commented Jun 4, 2020

Ah right. In my head skipchars was using the predicate the opposite of how it actually is. I'll remove those commits while I try to figure out the best way of using the deprecation machinery for this and let this PR be about eachof. Will file a separate PR for skipuntil later.

@non-Jedi non-Jedi changed the title Add eachof function for iterating streams and rename skipchars to skipuntil Add eachof function for iterating streams Jun 4, 2020
non-Jedi added 2 commits June 4, 2020 16:50
@non-Jedi
Add eachof function for iterating while reading from io
2cd7855 
This saves boilerplate versus a while !eof(io); read(io, T) loop. The
function signature is patterned after the read(io, T) function
signature.

Prompted by JuliaLang#36132 but does not close JuliaLang#36132

I am open to better/clearer names for this function.
@non-Jedi
Keep skipchars from allocating unnecessary String
28d66b1 
Remove usage of readline from skipchars definition.
non-Jedi added 2 commits June 4, 2020 16:53
@non-Jedi
Add NEWS entry for eachof
b0fac83
@non-Jedi
Refactor to make use of eachof function
96a3a52 
These are all the places in base/stdlib where I saw the pattern used:

while!eof(io); read(io, T)

I think they make a good case study on this function's ability to make
code dealing with IO cleaner.
@non-Jedi
Copy link
Contributor Author

non-Jedi commented Jun 5, 2020

aarch64 failure seems unrelated

@StefanKarpinski
Copy link
Member

Can I suggest the name readeach instead of eachof?

@non-Jedi
Copy link
Contributor Author

non-Jedi commented Jun 5, 2020

readeach seems much better than eachof. I anticipated some discussion of what the best name for this was, so I'm going to wait to make the change to see if there are other suggestions.

@non-Jedi
Copy link
Contributor Author

non-Jedi commented Jun 8, 2020

Would eachread be better than readeach for symmetry with eachline, eachindex, etc.?

@StefanKarpinski
Copy link
Member

I think readeach(io, T) sounds more natural, personally, so I'd go with that.

@non-Jedi non-Jedi changed the title Add eachof function for iterating streams Add readeach function for iterating streams Jun 18, 2020
tkf
tkf reviewed Jun 18, 2020
View reviewed changes
base/io.jl Outdated Show resolved Hide resolved
@tkf tkf added io Involving the I/O subsystem: libuv, read, write, etc. iteration Involves iteration or the iteration protocol labels Jun 18, 2020
@JeffBezanson
Copy link
Member

Triage says 👍 to readeach.

@JeffBezanson JeffBezanson removed the triage This should be discussed on a triage call label Jun 18, 2020
rfourquet
rfourquet reviewed Jun 19, 2020
View reviewed changes
base/io.jl Outdated Show resolved Hide resolved
base/io.jl Outdated Show resolved Hide resolved
base/io.jl Show resolved Hide resolved
StefanKarpinski
StefanKarpinski approved these changes Jun 19, 2020
View reviewed changes
Copy link
Member

@StefanKarpinski StefanKarpinski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

non-Jedi added 2 commits July 17, 2020 10:42
@non-Jedi
Revert substituting skipchars for readline
7ed76b2 
Although using skipchars reduced allocations, it overall slows down
the function.
@non-Jedi
Use IOBuffer for readeach example
e1ce5aa 
This is more succinct and focused than writing a file to disk and
using open.
@non-Jedi
Copy link
Contributor Author

I've pushed two commits addressing @rfourquet's review that can be squashed once this is ready for merge.

@StefanKarpinski StefanKarpinski added the merge me PR is reviewed. Merge when all tests are passing label Jul 18, 2020
@tkf
Copy link
Member

tkf commented Jul 19, 2020

It looks like thre is a conflict in NEWS file.

@StefanKarpinski StefanKarpinski merged commit 3a86b7d into JuliaLang:master Jul 23, 2020
simeonschaub pushed a commit to simeonschaub/julia that referenced this pull request Aug 11, 2020
@non-Jedi @mbauman @simeonschaub
Add readeach function for iterating streams (JuliaLang#36150)
7b289e6 
Co-authored-by: Matt Bauman <mbauman@juliacomputing.com>
@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Jan 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JeffBezanson JeffBezanson JeffBezanson left review comments

@tkf tkf tkf left review comments

@rfourquet rfourquet rfourquet approved these changes

@StefanKarpinski StefanKarpinski StefanKarpinski approved these changes

Assignees
No one assigned
Labels
io Involving the I/O subsystem: libuv, read, write, etc. iteration Involves iteration or the iteration protocol
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@non-Jedi @JeffBezanson @StefanKarpinski @tkf @rfourquet @DilumAluthge @mbauman