Add type assertion to Symbol method. Fix #242
#243
Conversation
| @@ -1160,6 +1160,8 @@ if VERSION ≥ v"0.4.0-dev+3732" | |||
| @test Symbol("a_", 2) === :a_2 | |||
| @test Symbol('c') === :c | |||
| @test Symbol(1) === Symbol("1") | |||
| # Behaviour on 0.5 might change JuliaLang/julia#7258 | |||
| @test keytype([Symbol(k) => v for (k,v) in Dict{Any, Any}(:x=>3)]) == Symbol | |||
There was a problem hiding this comment.
I don't think we should have this test
There was a problem hiding this comment.
Fixing things without any way of checking they don't break again doesn't sound like a good idea. We could limit it to 0.4 if you're afraid of it breaking because of changes in Julia.
There was a problem hiding this comment.
This is basically checking undefined behavior that happens to work.
There was a problem hiding this comment.
Yes, but it's not going to change on 0.4 at this point, as it would be a breaking change.
There was a problem hiding this comment.
FWIW, it is actually perfectly fine to add the test on 0.5 after JuliaLang/julia#16622. Before that, we really can't have any guarantee what the type is. To see how sensitive it is,
julia> Base.Symbol(a::Float64) = symbol(a)
Symbol
julia> Base.Symbol(a::Int) = symbol(a)
Symbol
julia> Base.Symbol(a::AbstractString) = symbol(a)
Symbol
julia> Base.Symbol(s::Symbol) = s
Symbol
julia> [Symbol(k) => v for (k,v) in Dict{Any, Any}(:x=>3)]
Dict{Any,Any} with 1 entry:
:x => 3And this is totally sensible thing to do if we need to slightly tweak how Symbol behaves (such that they don't exactly match symbol on 0.4).
I'm OK with leaving the test in given that we'll delete it if it breaks. I don't see the point of the test in that case though.
|
Bump. This doesn't hurt anything, does it? |
|
This lgtm, was mainly hoping to get a passing ci (which should be ok now?) |
|
I think there are other issues but they're being worked on in different PR's. |
|
We can maybe add the test back once the comprehensions thing merges to 0.5. |
* Add type assertion to `Symbol` method. Fix JuliaLang#242 * Remove comprehension type inference test
@yuyichao @nalimilan Tested DataFrames, PyCall and SciktiLearn on Julia 0.4.5, all tests passed.