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

Add Compat entries for #11140 & #11241 #90

Merged
merged 1 commit into from
May 27, 2015

Conversation

ScottPJones
Copy link
Contributor

This adds Compat entries for is_valid_utf32, and all of the isvalid changes.

# isvalid
let s = "abcdef", u8 = "abcdef\uff", u16 = utf16(u8), u32 = utf32(u8)
@test !isvalid(UTF32String(UInt32[65,66,0x110000,0])
@test !isvalid(Char(0x110000))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test needs to be something like @test !isvalid(@compat Char(0x110000)) for 0.3

@ScottPJones
Copy link
Contributor Author

Thanks, I've been trying to figure out how to make it work in 0.3

@ScottPJones
Copy link
Contributor Author

Should be fixed now!

@tkelman
Copy link
Contributor

tkelman commented May 22, 2015

Cool, I think this looks right. Thank you very much!

@ScottPJones
Copy link
Contributor Author

@tkelman Can this be merged now? (so nobody has issues going back and forth between 0.4 and 0.3.x?) Thanks!

@tkelman
Copy link
Contributor

tkelman commented May 27, 2015

Sure.

tkelman added a commit that referenced this pull request May 27, 2015
Add Compat entries for #11140 & #11241
@tkelman tkelman merged commit 63739de into JuliaLang:master May 27, 2015
@hayd
Copy link
Member

hayd commented May 28, 2015

Not sure this was quite right as it failed travis https://travis-ci.org/JuliaLang/Compat.jl/jobs/64189228

MethodError: `isvalid` has no method matching isvalid(::Type{UTF16String}, ::UTF16String)

Does isvalid etc need to be exported outside of the if-block?

@tkelman
Copy link
Contributor

tkelman commented May 28, 2015

Hm, that failed on 0.4.0-dev+5023, so it's actually that the test is trying to use methods that don't exist on Julia master

INFO: Testing Compat
WARNING: is_valid_utf32(str::UTF32String) is deprecated, use isvalid(str) instead.
 in depwarn at ./deprecated.jl:62
 in is_valid_utf32 at deprecated.jl:49
 in anonymous at test.jl:91
 in do_test at test.jl:49
 in include at ./boot.jl:252
 in include_from_node1 at loading.jl:133
 in process_options at ./client.jl:320
 in _start at ./client.jl:412
while loading /home/travis/.julia/v0.4/Compat/test/runtests.jl, in expression starting on line 304
ERROR: LoadError: test error in expression: isvalid(UTF16String,u16)
MethodError: `isvalid` has no method matching isvalid(::Type{UTF16String}, ::UTF16String)
Closest candidates are:
  isvalid(::Type{UTF16String}, ::AbstractArray{UInt16,N})
 in anonymous at test.jl:91
 in do_test at test.jl:49
 in anonymous at no file:320
 in include at ./boot.jl:252
 in include_from_node1 at loading.jl:133
 in process_options at ./client.jl:320
 in _start at ./client.jl:412
while loading /home/travis/.julia/v0.4/Compat/test/runtests.jl, in expression starting on line 309

@hayd
Copy link
Member

hayd commented May 28, 2015

That was the other possibility! It's annoying that this version number is not printed in the travis log, or available more cleanly from the commit without building (perhaps I'm missing a trick here?).

@hayd hayd mentioned this pull request May 28, 2015
@tkelman
Copy link
Contributor

tkelman commented May 28, 2015

I specifically made sure that the version number would be printed in the Travis log when we were implementing language: julia support, see https://travis-ci.org/JuliaLang/Compat.jl/jobs/64189228#L87

@hayd
Copy link
Member

hayd commented May 28, 2015

@tkelman sorry, I meant on the JuliaLang/julia travis log!

@tkelman
Copy link
Contributor

tkelman commented May 28, 2015

@hayd
Copy link
Member

hayd commented May 28, 2015

Hmmmmmmmmm, weirdly versioninfo does show the version number locally e.g. +5016... But on travis it just has the branch name:

$ /tmp/julia/bin/julia-debug -e 'versioninfo()'
Julia Version 0.4.0-dev
...

e.g. look for "versioninfo" on this build.

@tkelman
Copy link
Contributor

tkelman commented May 28, 2015

Oh, that is odd. I think what might be going on there is the way travis clones from github is confusing our version_git script, note the warnings a little earlier

fatal: No names found, cannot describe anything.
fatal: bad revision '^'

@ScottPJones
Copy link
Contributor Author

Did I do something wrong? :sad:

@tkelman
Copy link
Contributor

tkelman commented May 29, 2015

Will need to fix the method error from #90 (comment) - not sure why Travis didn't catch this the first time around

@hayd
Copy link
Member

hayd commented May 29, 2015

Travis didn't catch it cos it was Julia Version 0.4.0-dev+4850.

@ScottPJones
Copy link
Contributor Author

OK, I figured it out... my tests were on v0.3.7, v0.3.8 and v0.4 before isvalid... and this breaks now that isvalid has been added to master, and the Compat code isn't being used, because although the Compat code handled the two argument case, the code in master missed a method for the 2-argument form...
I'm testing the fix now, but it will need to be merged into julia/master (and who knows how long that will take!) The problem is only in the test though, not in the additions to Compat.jl themselves.
That does show an issue with Compat... how do you know it gives you anything at all like actual implementation of something in the later versions of julia? You just have to be very careful... but there are no cross-version tests...

@ScottPJones
Copy link
Contributor Author

Should be fixed by #11482 when/if that is merged...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants