Add coverage tests for strings/basic.jl #12898
Conversation
| # This gets a deprecation warning that seems incorrect | ||
| # getindex("a",1.0) -> WARNING: indexing with non Integer Reals is deprecated | ||
| # Capture spurious deprecation warning | ||
| let |
There was a problem hiding this comment.
This doesn't belong here. You should be testing current behavior, not the behavior that you may want.
There was a problem hiding this comment.
Do you mean the comment? It is testing current behavior already.
There was a problem hiding this comment.
You are testing depwarn behavior, just test the behavior that is implemented in Base.
There was a problem hiding this comment.
The behavior is implemented in Base. It showed up on the coverage tests.
Look at line 39 in strings/basic.jl:
getindex(s::AbstractString, x::Real) = s[to_index(x)]There was a problem hiding this comment.
I am not going to debate this, please remove the contents of this let block and I'll merge.
There was a problem hiding this comment.
Should the getindex call itself be moved to deprecated.jl then, since it depends on a deprecated method (to_index(x::Real))?
I was just trying to make sure that every line in strings was covered.
ScottPJones
force-pushed
the
spj/testbasic
branch
from
c8cbb69 to
5efdbe1
Compare
Remove testing of getindex/checkbounds
ScottPJones
force-pushed
the
spj/testbasic
branch
from
5efdbe1 to
a77cfe8
Compare
|
@jakebolewski I've removed them (I'm going to do another PR to remove the code that depended on deprecated code, here and in char.jl) |
|
Thanks! |
Add coverage tests for strings/basic.jl
These tests should hopefully bring the coverage of strings/basic.jl to 100%