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

Default to OrderedStruct for NamedTuples? #68

Open
FedorChervyakov opened this issue Dec 6, 2021 · 3 comments
Open

Default to OrderedStruct for NamedTuples? #68

FedorChervyakov opened this issue Dec 6, 2021 · 3 comments

Comments

@FedorChervyakov
Copy link

FedorChervyakov commented Dec 6, 2021

Would it be possible to define default StructType for NamedTuples as StructTypes.OrderedStruct()? Or are there any obvious conflicts I'm not seeing? It seems like the problems that might arise because of this are similar to those described in #61, but I am unable to come up with any other StructType for NamedTuples, that might be of use.

@quinnj
Copy link
Member

quinnj commented Dec 7, 2021

Is there a reason you'd prefer OrderedStruct vs UnorderedStruct? I've had an idea of possibly refactoring things slightly to allow per-module overloads for specific cases. You can already do this yourself, like we do in JSON3, with our various read overloads, but it could be nice to allow overloading directly. So you'd be able to do something like StructTypes.StructType(::Val{:MyModule}, ::Type{<:NamedTuple}) = StructTypes.UnorderedStruct() or something like that.

@zot
Copy link

zot commented Jan 28, 2022

[EDIT: simplified defs -- check PR for more details]

These definitions appear to work.

These allow obj to have fields that are not in T (issuing a @debug msg) but obj must contain all the fields in T except for those are allowed to be nothing or missing.

I'll make a pull request for this:

StructType(::Type{T}) where {T <: NamedTuple} = UnorderedStruct()

constructfrom(t::Type{NamedTuple}, obj::NamedTuple) =
    constructfrom(StructType(t), t, StructType(typeof(obj)), obj)

constructfrom(::Union{Struct, UnorderedStruct}, t::Type{T}, ::Union{Struct, UnorderedStruct}, obj::NamedTuple) where {T <: NamedTuple} =
     construct_nt(t, obj)

construct_nt(::Type{NamedTuple}, obj) = obj

function construct_nt(t::Type{T}, obj) where {T <: NamedTuple}
    keys = fieldnames(t)
    keyset = Set(keys)
    result = Dict()
    for (key, value) in pairs(obj)
        field = construct(Symbol, key)
        if field in keyset
            result[field] = constructfrom(fieldtype(t, field), value)
            delete!(keyset, field)
        else
            msg = "Extraneous field $field in $obj, not in type $t"
            try
                throw(ArgumentError(msg))
            catch err
                @debug msg exception=err,catch_backtrace()
            end
        end
    end
    for key in keyset
        type = fieldtype(t, key)
        if nothing isa type
            result[key] = nothing
        elseif missing isa type
            result[key] = missing
        else
            continue
        end
        delete!(keyset, key)
    end
    length(keyset) > 0 && throw(ArgumentError("Missing required fields $(join(keyset, ",")) from $obj in type $t"))
    t(result[key] for key in keys)
end

@ericphanson
Copy link

ericphanson commented Feb 7, 2022

Is there a reason you'd prefer OrderedStruct vs UnorderedStruct?

I've had this case for NamedTuples where I have a row-table like Vector{T} for some T <:NamedTuple, and I want to push! a new row onto it, and maybe either the table or the row was just deserialized from JSON. The order matters because the keys must be in the right order for the types to match to succesfully push!.

Nevermind, I forgot what OrderedStruct meant (it's about assumptions on JSON format, not whether or not we want the structs fields to be ordered correctly, which of course we always do for a struct).

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 a pull request may close this issue.

4 participants