quinnj / JSON3.jl

Other
214 stars 47 forks source link

When generating types, distinguish missing and nothing #166

Open mcmcgrath13 opened 3 years ago

mcmcgrath13 commented 3 years ago

Instead of treating a missing field as a Nothing type when generating types, instead use Missing. If the struct is mutable, default the fields with Missing types to missing on initialization. (Is there a better way to build that constructor?)

This can lead to a case where there's a Union{Missing, Nothing...} type in the generated structs, so I added a check in the read function for this case so that it prefers reading null into nothing over missing when both types are present.

I'm not sure this is the right approach, but wanted to start the PR to discuss.

closes #143

codecov[bot] commented 3 years ago

Codecov Report

Merging #166 (7fbfb06) into main (1c69202) will increase coverage by 0.05%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #166      +/-   ##
==========================================
+ Coverage   88.76%   88.81%   +0.05%     
==========================================
  Files           9        9              
  Lines        1531     1538       +7     
==========================================
+ Hits         1359     1366       +7     
  Misses        172      172              
Impacted Files Coverage Δ
src/gentypes.jl 98.37% <100.00%> (+0.09%) :arrow_up:
src/structs.jl 90.00% <100.00%> (ø)

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 3d9d086...7fbfb06. Read the comment docs.

quinnj commented 3 years ago

Hmmmm, I don't know that I love this use of Missing when a field/value is missing (I know, I know, pun not intended). What about the approach of still using Nothing for fields that are not present, but using Some{T} when there's a mix of null and other values?

mcmcgrath13 commented 3 years ago

Hmmmm, I don't know that I love this use of Missing when a field/value is missing (I know, I know, pun not intended). What about the approach of still using Nothing for fields that are not present, but using Some{T} when there's a mix of null and other values?

So would the types of those fields be Union{Some{T}, Nothing} then initialized to nothing if mutable (and T could be a union type that includes Nothing)? As I understand it missing data is pretty incompatible with the immutable case due to how those structs are constructed.

A toy example:

[{"a": null, "b": 1},
{"b": null},
{"a": 1, "b": 3}]

would turn into:

mutable struct Root
  a::Union{Some{Union{Int, Nothing}}, Nothing} # both might be missing and have null data
  b::Union{Int,Nothing} # only null data
  function Root()
    x = new()
    x.a = nothing
  end
end

This feels a little clunky to me as the a field needs to be unwrapped whereas b doesn't, though I suppose there's no harm in unwrapping b too if someone wanted to.

How widely used is Some? I haven't come across it much.

nstiurca commented 1 year ago

Regarding the approach, I think one problem is there is no general consensus in the Julia community on how to handle missing values. Different projects do it different. Doubly so for projects which get their json data from other projects which may have different opinions, which I suspect is quite common. At any rate, I think the user should have a way to configure the behavior for their structs, probably via StructTypes.jl.