Restrict signature for getindex(::AbstractString, ::AbstractVector} #20248
Conversation
| @@ -461,3 +460,7 @@ Base.endof(x::CharStr) = endof(x.chars) | |||
| # Case with Unicode characters | |||
| @test cmp("\U1f596\U1f596", CharStr("\U1f596")) == 1 # Gives BoundsError with bug | |||
| @test cmp(CharStr("\U1f596"), "\U1f596\U1f596") == -1 | |||
|
|
|||
| # issue #12495: logical indexing attempt raises MethodError | |||
There was a problem hiding this comment.
Comment here should say ArgumentError?. It might also be good to be a bit more explicit, for example isue #12495: check that logical indexing attempt raises MethodError so we know if the problem in #12495 was that logical indexing raises the error or that the test is checking that the correct error is being raised.
| @@ -461,3 +460,7 @@ Base.endof(x::CharStr) = endof(x.chars) | |||
| # Case with Unicode characters | |||
| @test cmp("\U1f596\U1f596", CharStr("\U1f596")) == 1 # Gives BoundsError with bug | |||
| @test cmp(CharStr("\U1f596"), "\U1f596\U1f596") == -1 | |||
|
|
|||
| # issue #12495: logical indexing attempt raises MethodError | |||
There was a problem hiding this comment.
it's throwing an argumenterror
There was a problem hiding this comment.
Good catch, I first thought about throwing MethodError but then realized it would be confusing. Fixed.
Only integer indices are supported by the generic interface, so better raise a MethodError for other element types rather than failing later. Ideally the Bool-specific method would not be needed, but it is required as long as Bool<:Integer.
|
Will merge tomorrow if nobody objects. |
|
Should this have been a deprecation? Any packages using it? |
|
These cases already failed, this PR merely made the error explicit. The only forms which were supported before and now fail are things like |
…gnature tightening (JuliaLang#20248).
Only integer indices are supported by the generic interface, so better
raise a MethodError for other element types rather than failing later.
Ideally the Bool-specific method would not be needed, but it is required
as long as Bool<:Integer.
Fixes #12495.
(Some of the tests that needed modifications look really weird to me.)