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

Implementing a string method that works for iterators and generators #57180

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions base/strings/util.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1274,3 +1274,29 @@ function Base.rest(s::AbstractString, st...)
end
return String(take!(io))
end
"""
The `String` constructor is enhanced to accept iterators/generator objects.

### Method Details:
- **String(x::AbstractIterator)**
- Converts an iterator into a string.
- Throws a `MethodError` if the iterator contains invalid data types (non-Char types) or if it is an infinite iterator.
- Ensures that the result is a valid string representation composed solely of characters (`Char`).
Comment on lines +1282 to +1285
Copy link
Member

Choose a reason for hiding this comment

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

Let's just make this a concise one-liner, which is more efficient than your version too (since it needs to make fewer intermediate copies):

String(x) = sprint(join, x)
Suggested change
- **String(x::AbstractIterator)**
- Converts an iterator into a string.
- Throws a `MethodError` if the iterator contains invalid data types (non-Char types) or if it is an infinite iterator.
- Ensures that the result is a valid string representation composed solely of characters (`Char`).
- **String(any_iterable)**
- prints an iterable object into a string using `join`.

Copy link
Contributor

Choose a reason for hiding this comment

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

But wait, do we want this? This has completely different semantics from the existing String function. For example, with this change we get:

julia> String(Int8[101, 102])
"101102"

julia> String(UInt8[101, 102])
"ef"

To me this seems like the wrong meaning of the type constructor String. This should only be used to convert things that are already a little bit 'string-like' and should not be conflated with print, which this implementation suggests.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, that makes sense. This seems more like write then:

String(x) = sprint(io -> foreach(c -> write(io, c), x))

The print behavior was from looking at the implementation of String(::AbstractVector{<:AbstractChar}), but that probably also can also be calling write, since it is a case where print is defined to be a call to write.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is a good idea, Examples Char(0x2ee8) and String([0x2ee8]) should be consistent. Also the dependency on Litte/Big-Endiness is not expected.
I think the prototype should be like ... String(collect(Char, x)) as I mentioned before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, so the problem with making just one liners is the error statements end up being loose for cyclic iterators, or even integers in some cases. So sticking with the ... String(collect(Char, x)) prototype.


### Examples
```jldoctest
julia> String(Iterators.map(c -> c+1, "Hello, world"))
"Ifmmp-!xpsme" # Generates a string by incrementing ASCII values of each character.

julia> String(Iterators.take("Hello, world", 5))
"Hello" # Takes the first 5 characters of the string and converts it to a string.
"""
String(x) = String_iterator(x, IteratorSize(x))
Copy link
Contributor

Choose a reason for hiding this comment

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

The name String_iterator does not follow the guidelines for Julia names. As you want to create an internal name (not exported), it could be _string_iterator, for example.

String_iterator(x, ::IsInfinite) = throw(MethodError(String, (x,)))
String_iterator(x, ::IteratorSize) = begin
collected = collect(x)
if !(isa(collected, AbstractVector) && all(x -> isa(x, Char), collected))
throw(MethodError(String, (x,)))
end
return String(collected::AbstractVector{<:AbstractChar})
Copy link
Contributor

Choose a reason for hiding this comment

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

You cannot guarantee that collected is an AbstractVector{<:AbstractChar}, even if it only contains Char.

Also, I'm not too fond of throwing a method error here. The method does exist, but the data may not be applicable. Consider an iterator that may return Char or Nothing. In this case, it will only throw a method error sometimes.

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about collected = collect(Char, itr) ? That would avoid if statement with the the all( isa) call, which requires an extra wipe through the data.
The call to collect guarantees an Array{Char,N} return value, where N == 1 if the iterator is 1-dimensional, N == 0, if the "iterator" is a number.

The String_iterator and call of IteratorSize would be avoided altogether by this:

String(itr) = String(collect(Char, itr))
String(x::AbstractArray{Char}) = throw(MethodError(String, (x, )))

end
71 changes: 71 additions & 0 deletions test/strings/util.jl
Original file line number Diff line number Diff line change
Expand Up @@ -791,3 +791,74 @@
@test endswith(A, split(B, ' ')[end])
@test endswith(A, 'g')
end
@testset "String Iterator Tests" begin
# Test with various types of iterators (String, SubStr, and GenericString)
for S in (String, SubStr, Test.GenericString)
# Valid iterators (strings, substrings, and characters)
@test String(Iterators.map(c -> c+1, "abc")) == "bcd"
@test String(Iterators.take("hello world", 5)) == "hello"
@test String(Iterators.filter(c -> c != ' ', "hello world")) == "helloworld"
@test String(Iterators.drop("hello world", 6)) == "world"

Check warning on line 802 in test/strings/util.jl

View workflow job for this annotation

GitHub Actions / Check whitespace

Whitespace check

trailing whitespace
# Single character iterators
@test String(Iterators.map(c -> 'a', "hello")) == "aaaaa"

Check warning on line 805 in test/strings/util.jl

View workflow job for this annotation

GitHub Actions / Check whitespace

Whitespace check

trailing whitespace
# Mixed characters and Unicode
@test String(Iterators.map(c -> c == ' ' ? ' ' : 'A', "hello world")) == "AAAAA AAAAA"
@test String(Iterators.map(c -> '😊', "hello")) == "😊😊😊😊😊"

# Edge cases: empty string or iterator with no elements
@test String(Iterators.take("", 3)) == ""
@test String(Iterators.drop("", 3)) == ""

Check warning on line 813 in test/strings/util.jl

View workflow job for this annotation

GitHub Actions / Check whitespace

Whitespace check

trailing whitespace
# Invalid iterators (non-Char elements in the iterator)
@test_throws MethodError String(Iterators.map(c -> 1, "hello"))
@test_throws MethodError String(Iterators.filter(c -> c > 128, "hello"))

# Infinite iterators (should raise MethodError)
@test_throws MethodError String(Iterators.cycle("abc"))

# Ensure valid behavior for non-iterable inputs
@test_throws MethodError String(19)
@test_throws MethodError String(3.14)

Check warning on line 824 in test/strings/util.jl

View workflow job for this annotation

GitHub Actions / Check whitespace

Whitespace check

trailing whitespace
# Mixed types, ensure correct character handling
@test String(Iterators.map(c -> Char(c), "abc")) == "abc"

Check warning on line 827 in test/strings/util.jl

View workflow job for this annotation

GitHub Actions / Check whitespace

Whitespace check

trailing whitespace
# Nested iterators (iterators within iterators)
@test String(Iterators.flatten(Iterators.map(c -> c, ["hello", "world"]))) == "helloworld"
@test String(Iterators.flatten(Iterators.map(c -> "hello", 1:3))) == "hellohellohello"

Check warning on line 831 in test/strings/util.jl

View workflow job for this annotation

GitHub Actions / Check whitespace

Whitespace check

trailing whitespace
# Using an empty generator that produces no values
@test String(Iterators.filter(c -> false, "hello")) == ""

Check warning on line 834 in test/strings/util.jl

View workflow job for this annotation

GitHub Actions / Check whitespace

Whitespace check

trailing whitespace
# Iterators with edge cases (multiple empty spaces, mixed Unicode chars)
@test String(Iterators.map(c -> ' ', "hello world")) == " "
@test String(Iterators.map(c -> '😊', "hi there")) == "😊😊😊😊😊😊😊😊"

Check warning on line 838 in test/strings/util.jl

View workflow job for this annotation

GitHub Actions / Check whitespace

Whitespace check

trailing whitespace
# Checking for correct error handling with invalid iterators
@test_throws MethodError String(Iterators.map(c -> [1,2], "hello"))
end

Check warning on line 842 in test/strings/util.jl

View workflow job for this annotation

GitHub Actions / Check whitespace

Whitespace check

trailing whitespace
# Additional tests for infinite iterators
@test_throws MethodError String(Iterators.cycle("abc"))

Check warning on line 845 in test/strings/util.jl

View workflow job for this annotation

GitHub Actions / Check whitespace

Whitespace check

trailing whitespace
# Ensure that String is only created from valid iterators (chars, strings)
@test String("valid string") == "valid string"
@test String("test") == "test"

# Edge case with non-character data in the iterator
@test_throws MethodError String(Iterators.map(c -> 42, "hello"))

# Nested iterator tests (flattening iterators)
@test String(Iterators.flatten(Iterators.map(c -> "abc", 1:3))) == "abcabcabc"
@test String(Iterators.flatten(Iterators.map(c -> "😊", 1:2))) == "😊😊"

# Edge case with no characters in the iterator
@test String(Iterators.filter(c -> false, "hello")) == ""
@test String(Iterators.take("hello", 0)) == ""

# Performance testing for large input
@test String(Iterators.take("a"^1000, 500)) == "a"^500
@test String(Iterators.map(c -> 'a', "a"^100000)) == "a"^100000
Priynsh marked this conversation as resolved.
Show resolved Hide resolved
end
Loading