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

REPL doc lookup assumed ASCII for the given string #41594

Merged
merged 3 commits into from
Jul 15, 2021

Conversation

Seelengrab
Copy link
Contributor

Fix #41589

Original code assumed ASCII, but that's not true. There may be other places in Base or an Stdlib where this happens, I'll grep and possibly open another PR if I find more.

Also, is there a better way to check whether something just doesn't throw? Checking the type of the returned value certainly seems like a way, but is of limited use for other code paths and kind of feels wrong...

Original code assumed ASCII, but that's not true.
@KristofferC KristofferC added the merge me PR is reviewed. Merge when all tests are passing label Jul 15, 2021
@jakobnissen
Copy link
Contributor

jakobnissen commented Jul 15, 2021

I'm pretty sure the indices 1:end-1 works for any string that ends in a =, so I'm not convinced the old behaviour is incorrect.
Edit: You are right and I was wrong.

@louie-github
Copy link

A quick test shows this not to be the case:

julia> "÷="[end-1]
ERROR: StringIndexError: invalid index [2], valid nearby indices [1]=>'÷', [3]=>'='
Stacktrace:
 [1] string_index_err(s::String, i::Int64)
   @ Base .\strings\string.jl:12
 [2] getindex_continued(s::String, i::Int64, u::UInt32)
   @ Base .\strings\string.jl:233
 [3] getindex(s::String, i::Int64)
   @ Base .\strings\string.jl:226
 [4] top-level scope
   @ REPL[1]:1

stdlib/REPL/src/docview.jl Outdated Show resolved Hide resolved
stdlib/REPL/test/docview.jl Outdated Show resolved Hide resolved
@fredrikekre fredrikekre added backport 1.6 Change should be backported to release-1.6 backport 1.7 docsystem The documentation building system REPL Julia's REPL (Read Eval Print Loop) labels Jul 15, 2021
@Seelengrab
Copy link
Contributor Author

Seelengrab commented Jul 15, 2021

I'm pretty sure the indices 1:end-1 works for any string that ends in a =, so I'm not convinced the old behaviour is incorrect.

The byte index before a valid Char index may not be a valid Char index itself (except for single-byte encodings like ASCII). In this case, the character in front of = is a multi-byte character of length 2 bytes, that's why end - 1 is not valid here.


Thanks for the quick review, did not know about chop! Should there be a comment next to the test, saying that the isa Markdwon.MD check is just used for confirming that it doesn't throw, instead of requiring that return type?

@KristofferC
Copy link
Member

Should there be a comment next to the test, saying that the isa Markdwon.MD check is just used for confirming that it doesn't throw, instead of requiring that return type?

I don't think it really matters. Someone can always follow the issue -> PR trail from the testset name if they are really interested.

@fredrikekre fredrikekre merged commit be443ac into JuliaLang:master Jul 15, 2021
@Seelengrab Seelengrab deleted the unicode_doc_idx branch July 15, 2021 11:09
@KristofferC KristofferC mentioned this pull request Jul 19, 2021
75 tasks
KristofferC pushed a commit that referenced this pull request Jul 19, 2021
KristofferC pushed a commit that referenced this pull request Jul 19, 2021
@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Jul 20, 2021
KristofferC pushed a commit that referenced this pull request Jul 20, 2021
KristofferC pushed a commit that referenced this pull request Jul 20, 2021
KristofferC pushed a commit that referenced this pull request Aug 31, 2021
@KristofferC KristofferC removed the backport 1.6 Change should be backported to release-1.6 label Sep 7, 2021
staticfloat pushed a commit that referenced this pull request Dec 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docsystem The documentation building system REPL Julia's REPL (Read Eval Print Loop)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Help/documentation for ÷= is broken
6 participants