quinnj / JSON3.jl

Other
214 stars 47 forks source link

Generate types from an array of JSON samples #145

Closed kskyten closed 3 years ago

kskyten commented 3 years ago

This is @mcmcgrath13's suggestion from #142. It seems to be working, but the tests break for some reason. I didn't quite figure out why yet. The JSONTypes module gets replaced several times during the tests, so I think it has something to do with including the generated code. The generated code itself looks OK.

codecov[bot] commented 3 years ago

Codecov Report

Merging #145 (b406718) into main (c73edd7) will increase coverage by 0.04%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #145      +/-   ##
==========================================
+ Coverage   88.87%   88.91%   +0.04%     
==========================================
  Files           9        9              
  Lines        1528     1534       +6     
==========================================
+ Hits         1358     1364       +6     
  Misses        170      170              
Impacted Files Coverage Δ
src/gentypes.jl 98.36% <100.00%> (+0.08%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c73edd7...b406718. Read the comment docs.

mcmcgrath13 commented 3 years ago

It looks like the tests are passing to me, a few thoughts/questions:

  1. could you move up the read_json_str function above the generatetypes doc string and then also use it in the other method of generatetypes?
  2. What would we expect something like the below should produce for a type? Right now it errors
    weird_jsons =
            [
                "{\"a\": 1, \"b\": 2, \"c\": {\"d\": 4}}",
                "[{\"a\": \"w\"}, {\"b\": 5}, {\"c\": {\"d\": 4}}]",
                "{\"x\": 7}",
            ]

    At the root of a JSON struct, I'd want the unification of a vector of named tuples and a named tuple to just be the named tuple, but if it's nested, I don't think that works anymore and the union would be needed. How much "type" stability can we reasonable expect of JSON? At what point is erroring the sensible solution?

kskyten commented 3 years ago

The tests are actually passing for me as well. I was confused by the formatting a bit. It looked like some of the tests were not being run because of the warnings. I don't like the warnings of replacing the modules though.

The type stability of the samples is a good question. Did you test your example after #144 was merged?

kskyten commented 3 years ago

OK, I rebased it on top of #144. Your example doesn't error, but the output is not right either: The Root structs should be given distinct names.

julia> JSON3.generatetypes(weird_jsons, :JSONTypes; mutable=false)
:(module JSONTypes
  import StructTypes
  struct Root
      x::Int64
  end
  struct C
      d::Int64
  end
  struct Root
      a::Int64
      b::Int64
      c::C
  end
  struct C
      d::Int64
  end
  struct Root
      a::Union{Nothing, String}
      b::Union{Nothing, Int64}
      c::Union{Nothing, C}
  end
  StructTypes.StructType(::Type{Root}) = begin
          #= none:1 =#
          StructTypes.Struct()
      end
  StructTypes.StructType(::Type{C}) = begin
          #= none:1 =#
          StructTypes.Struct()
      end
  StructTypes.StructType(::Type{Root}) = begin
          #= none:1 =#
          StructTypes.Struct()
      end
  StructTypes.StructType(::Type{C}) = begin
          #= none:1 =#
          StructTypes.Struct()
      end
  StructTypes.StructType(::Type{Root}) = begin
          #= none:1 =#
          StructTypes.Struct()
      end
  end)
mcmcgrath13 commented 3 years ago

I'd argue there should only be one root struct, and/or we'd need to think about sum types for the root. When passing the JSON to JSON3 to parse into the type, you'd need to know which root was the right one in the current case (assuming a rename).

If we add a function like (seems like there should be a built in way to do this, but eltype doesn't work with named tuples as needed here):

type_or_eltype(::Type{Vector{T}}) where {T} = T
type_or_eltype(::Type{T}) where {T} = T

Then also broadcast the raw types over that before unifying (reduce(unify, type_or_eltype.(generate_type.(json)); init=Top)), the resulting type is a bit more sane.

We still might need to catch the edge case where a union of a vector of a type and a type (Union{Vector{T}, T}) both spit out a type definition, which may or may not be exactly the same.

mcmcgrath13 commented 3 years ago

Just checking in on this PR. @kskyten do you want to finish it out? I can also merge it into another development branch and get it over the finish line.

mcmcgrath13 commented 3 years ago

Thanks for getting this started @kskyten ! I'm merging this into another dev branch and I'll get it over the finish line.