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

Make startswith work with IO objects #43055

Merged
merged 9 commits into from
Feb 14, 2023
Merged

Conversation

ArunS-tack
Copy link
Contributor

closes #40616.

@JeffBezanson JeffBezanson added needs news A NEWS entry is required for this change needs tests Unit tests are required for this change labels Nov 12, 2021
@JeffBezanson
Copy link
Sponsor Member

I suppose this should use mark and reset?

@JeffBezanson JeffBezanson added the domain:io Involving the I/O subsystem: libuv, read, write, etc. label Nov 12, 2021
base/strings/util.jl Outdated Show resolved Hide resolved
@ArunS-tack
Copy link
Contributor Author

I suppose this should use mark and reset?

I am not quite familiar with those. Where should I add them?

base/strings/util.jl Outdated Show resolved Hide resolved
Co-authored-by: Steven G. Johnson <stevenj@mit.edu>
test/strings/basic.jl Outdated Show resolved Hide resolved
stevengj
stevengj previously approved these changes Dec 18, 2022
@stevengj stevengj removed the needs tests Unit tests are required for this change label Dec 18, 2022
@stevengj stevengj dismissed their stale review December 18, 2022 18:41

Still needs NEWS item

@stevengj
Copy link
Member

Looks like it just needs a NEWS item, something like:

* `startswith` now supports seekable `IO` streams ([#43055]).

@KristofferC
Copy link
Sponsor Member

KristofferC commented Dec 18, 2022

Personally, I think this is kind of a strange API for an IO. For example, in the implementation here you need to support mark and reset but those are not actually needed to check if an IO "starts with" something. In the same vein, there are many other "stringy" type of functions (contains, endswith, etc) that one could theoretically call on an IO because you can always materialize a String from an `IO´ and then reset but that doesn't mean it is a good idea to add all these.

But I think it is best to keep IO to be used for reading/writing and then you just compose that with the string functions to get the desired behavior. That avoids many questions like what state the IO object should be in after this function is called and makes it up to the user to decide on a case by case basis.

@stevengj
Copy link
Member

As @schneiderfelipe pointed out in the original issue, it's a nice complement to peek — indeed, checking if the subsequent stream starts with something is the basic use case for peek, and this is both a generalization that and arguably nicer API for that kind of query.

@KristofferC KristofferC added the status:merge me PR is reviewed. Merge when all tests are passing label Feb 13, 2023
Check if an `IO` object starts with a prefix.
"""
function Base.startswith(io::IO, prefix::Base.Chars)
mark(io)
Copy link
Member

@stevengj stevengj Feb 13, 2023

Choose a reason for hiding this comment

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

Seems worth it to optimize the common case of an ASCII char, especially since a stream may conceivably support peek but not mark/reset.

Suggested change
mark(io)
prefix isa AbstractChar && prefix '\x7f' && return peek(io) == prefix % UInt8
mark(io)

base/strings/util.jl Outdated Show resolved Hide resolved
@KristofferC KristofferC removed the needs news A NEWS entry is required for this change label Feb 14, 2023
@KristofferC KristofferC merged commit 1c5b80f into JuliaLang:master Feb 14, 2023
@ArunS-tack ArunS-tack deleted the iosw branch February 14, 2023 17:54
@DilumAluthge DilumAluthge removed the status:merge me PR is reviewed. Merge when all tests are passing label Feb 16, 2023
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make startswith work with IO objects
5 participants