-
Notifications
You must be signed in to change notification settings - Fork 18
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
move DirectIndexString from Base #24
Conversation
src/LegacyStrings.jl
Outdated
@@ -97,4 +98,11 @@ using Compat | |||
else | |||
include("rep.jl") | |||
end | |||
|
|||
if isdefined(Base, :DirectIndexString) |
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 think you'll have to define this earlier, since DirectIndexString
is used above. You'll be able to check that once you'll have a version of Julia without it.
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.
moved
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.
Looks good to me, the code is tested via ASCIIString
. Though we should probably wait until JuliaLang/julia#24259 has been merged to check that tests pass without DirectIndexString
in Base.
EDIT: the problem is, tests currently fail on 0.7...
Looks good to me as well. I assume the UTF16 |
Yes - the error seems to be line 763 of |
Are there tests for |
I have moved everything from Base that referenced This part of code:
seems to have been testing |
We should try to get this merged and tagged ASAP to avoid a gap in availability on 0.7. Any ideas about the 0.7 failures, @nalimilan? |
Honestly I don't think anybody cares much about I've traced the failure to |
I have a fix at #25. |
|
PR following JuliaLang/julia#24259