-
-
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
Add get(string, index, default)
#22500
Conversation
base/strings/basic.jl
Outdated
@@ -39,6 +39,7 @@ getindex(s::AbstractString, v::AbstractVector{<:Integer}) = | |||
getindex(s::AbstractString, v::AbstractVector{Bool}) = | |||
throw(ArgumentError("logical indexing not supported for strings")) | |||
|
|||
get(s::AbstractString, i::Integer, default) = start(s) <= i <= endof(s) ? s[i] : default |
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 would make the index check isvalid(s, i)
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.
Nice, I tried checkbounds()
but that throws an error. isvalid()
works great.
66a7df7
to
0aadeaa
Compare
Good idea. Can you add a test? |
test/strings/basic.jl
Outdated
# issue #22500 (using `get()` to safely index strings) | ||
@test get("Julia", 1, ' ') == 'J' | ||
@test get("Julia", -1, ' ') == ' ' | ||
@test get("Julia", 10, ' ') == ' ' |
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.
Probably worth testing that the default is returned when indexing at a position which is within the bounds of the string, but doesn't correspond to the beginning of a character?
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.
Excellent. This is exactly the kind of sideways thinking I want when I open a PR. Addressed in my new squashed commit.
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.
Cool! But there's some spacing issue which made the CI fail.
0aace7f
to
314ec66
Compare
314ec66
to
d4b5b5c
Compare
This feels a bit undermotivated to me. Was there an actual use case here? |
I don't remember the original use case for this, but it seems a good tool for defensive programming. It allows me to do things like write verbs such as "split paragraph X into sentences, then take the 2nd character of the 3rd word" in a way that allows me to treat failure paths the same between characters in a word vs. words in a sentence. |
Eh, sure. If there's even a vague motivation for it, I guess it's ok. It treats indexing into the middle of a character the same as out-of-bounds indexing, however, which seems pretty sketchy to me. |
While that's true, the purpose behind |
I don't think that's quite true – there's a difference between asking about a nonsense index and asking about an index that makes sense but doesn't exist. Again, an actual motivating use case would help understand what to do here. |
I agree with you; these are two different classes with errors that have different meanings. What I'm saying is that if your API only takes in I agree that indexing into the middle of wide characters is fundamentally a very different failure mode than indexing off the end of a string, but from the Seeing as this is a pretty simple case, I would suggest that if someone wants to differentiate between the different errors, they should just deal with those directly, and not use the |
For what it's worth
|
I think we should know the concrete use case before making a decision. In general, I don't see in what case it would be legitimate to call |
I changed this in #24999: it now returns the default if you use an out-of-bounds index and throws an error if you use an invalid, in-bounds index. |
I think it would be nice to provide this functionality with a wider set of collections. With this change we now get: