ascii: only support checking String for pure ASCIIness #16396
Conversation
|
|
||
| function ascii(s::String) | ||
| for b in s.data | ||
| b < 0x80 || throw(ArgumentError("invalid ASCII sequence")) |
There was a problem hiding this comment.
should we show the string?
There was a problem hiding this comment.
+1, show the position where the invalid char was found and print it.
|
should add a test that it throws on non-ascii input (unless we already have one that I can't find?), and docstrings need updating |
|
What's up with the test failure on AppVeyor? It looks like it succeeded and then hung. |
StefanKarpinski
force-pushed
the
sk/highlander2
branch
from
dd986ee to
7d3b32d
Compare
| @@ -8719,29 +8719,14 @@ alias the input `x` to modify it in-place. | |||
| filt! | |||
|
|
|||
| """ | |||
| ascii(::Array{UInt8,1}) | |||
| ascii(s::AbstractString) | |||
There was a problem hiding this comment.
make corresponding change in rst too
There was a problem hiding this comment.
would be good to migrate the remaining docstring inline
There was a problem hiding this comment.
Doesn't that get automatically done when the RST gets regenerated?
There was a problem hiding this comment.
This is what the script uses to find the correct place to insert the docstring so changing the signature will make the doc generation fail.
There was a problem hiding this comment.
The signatures now match...
There was a problem hiding this comment.
By inline I mean moving the markdown docstring to the source, so we can gradually get rid of this Base.jl file.
There was a problem hiding this comment.
You also should have run genstdlib and committed that at the same time, otherwise unrelated doc changes are ending up in multiple other PR's.
|
Any thoughts on whether I should merge this now and do additional work in a new PR or just add commits here? By additional work, I mean more parts of the |
|
|
||
| function ascii(s::String) | ||
| for (i, b) in enumerate(s.data) | ||
| b < 0x80 || throw(ArgumentError("invalid ASCII at index $i in $(repr(s))")) |
There was a problem hiding this comment.
do we have or want a more specific exception type than ArgumentError for this? I guess we have UnicodeError but are you planning on removing that?
|
I like the idea of keeping them small and separate if possible. Easier to review in smaller pieces. |
StefanKarpinski
changed the title
Part of #16107.