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

Fix JSON.parse with dicttype specified #137

Merged
merged 2 commits into from
Mar 28, 2016
Merged

Fix JSON.parse with dicttype specified #137

merged 2 commits into from
Mar 28, 2016

Conversation

ghost
Copy link

@ghost ghost commented Feb 26, 2016

No description provided.

@kmsquire
Copy link
Contributor

@maartenheremans, can you clarify the issue that this is addressing, and add a test if needed?

@Chief-PITA
Copy link
Contributor

@kmsquire The problem is that JSON.parse is supposed to be able to accept a dicttype parameter, however, it doesn't work if you want to specify, for example, Dict{Symbol, Any}, or anything with the type parameters all ready filled in. It seems that it was only tested for specifying an unparameterized type, such as OrderedDict, which it then always parameterizes as {AbstractString, Any}.
Note: there is also a bug, caused by JuliaLang/julia#13412, because the code uses the fptr field, both in src/JSON.jl and in test/runtests.jl.

@@ -245,7 +246,8 @@ end
@test sprint(JSON.print, [Inf]) == "[null]"

# Check printing of more exotic objects
@test sprint(JSON.print, sprint) == string("\"function at ", sprint.fptr, "\"")
# Test broken in v0.5, code is using internal structure of Function type!
#@test sprint(JSON.print, sprint) == string("\"function at ", sprint.fptr, "\"")
Copy link
Contributor

Choose a reason for hiding this comment

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

if this can't be adjusted in a more general way, then it should probably at least be conditional on the version where the change was merged

Copy link
Contributor

Choose a reason for hiding this comment

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

How can I track down the version string for that? I know the commit number: 9bfd27bd380124174a5f37c342e5c048874d71a4, but I don't know how to go from there to:
if VERSION < v"0.5.0-dev+1343". As far as adjusting it in a general way, Jeff has said it shouldn't even be trying to print out the function pointer for JSON.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you, that was perfect, v"0.5.0-dev+2396"

Update tests, check if no parameter for key set

Work around bug using .fptr from Function structure

Don't use keytype, which is broken in v0.4.3

Base code and test off of version string of PR 13412
@Chief-PITA
Copy link
Contributor

It seems there is another bug, on v0.4 and v0.3, the keytype function doesn't work correctly. I worked around it by directly accessing dictT.parameter[1], but it might be nice to get a fix for keytype done in v0.4 and possible v0.3, when it was introduced.

@nalimilan
Copy link
Member

What do you mean by "it doesn't work correctly"? Could you provide an example?

@Chief-PITA
Copy link
Contributor

Sorry for not being complete (the train I was waiting for arrived!). keytype (and valtype) were added in v0.4 (I thought I saw somewhere that it was in v0.3, but I was wrong), however it only has one definition,
for passing a value of an associative type, instead of the type itself:

julia> methods(keytype)
# 1 method for generic function "keytype":
keytype{K,V}(::Associative{K,V}) at dict.jl:219

and gets an error if you try to pass in the type, which works in v0.5:

julia> keytype(Dict{Int,Any})
ERROR: MethodError: `keytype` has no method matching keytype(::Type{Dict{Int64,Any}})

(In v0.5:)

julia> keytype(Dict{AbstractString,Any})
AbstractString

keytype{K,V}(::Type{Associative{K,V}}) at dict.jl:228
keytype(a::Associative{K<:Any,V<:Any}) at dict.jl:229
keytype{A<:Associative{K,V}}(::Type{A}) at dict.jl:230

@Chief-PITA
Copy link
Contributor

@kmsquire @nalimilan This is now passing all its tests in v0.3.x, v0.4.3, and master. Anything else? Thanks!

@enzobrands
Copy link

@kmsquire, @nalimilan, @tkelman Can this one be merged or are there further changes necessary?

@ghost
Copy link
Author

ghost commented Mar 25, 2016

@kmsquire , @nalimilan , @tkelman
Please let us know wether any changes are still required. We need this change in production code and would not like to maintain a separate fork for this

@tkelman tkelman merged commit f40303b into JuliaIO:master Mar 28, 2016
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.

5 participants