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

When generating types, distinguish missing and nothing #166

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mcmcgrath13
Copy link
Collaborator

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
Copy link

codecov bot commented Jun 26, 2021

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%) ⬆️
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
Copy link
Owner

quinnj commented Jul 6, 2021

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
Copy link
Collaborator Author

mcmcgrath13 commented Jul 6, 2021

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.


@test res[3].d == 10
@test ismissing(res[2].d)
@test res[1].d === nothing

Choose a reason for hiding this comment

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

Nit: can we reorder the asserts to be the same order as the input data, and also check for missing and nothing in the same way? Eg,

@test res[1].d === nothing
@test res[2].d === missing
@test res[3].d == 10

@nstiurca
Copy link

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.

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.

Distinguishing between different null types
3 participants