beacon-biosignals / Legolas.jl

Tables.jl-friendly mechanisms for constructing, reading, writing, and validating Arrow tables against extensible, versioned, user-specified schemas.
Other
24 stars 2 forks source link

Accessors.jl support (lack of positional argument constructors) #97

Closed kleinschmidt closed 1 year ago

kleinschmidt commented 1 year ago

The optics-based Accessors.jl makes it easy to "set" fields of immutable structs, and could replace a lot of Tables.rowmerge usage for legolas-defined record types. But because teh record types don't have positional argument constructors defined, @set and friends don't work:

julia> using Accessors

julia> using Legolas: @schema, @version

julia> @schema "foo" Foo

julia> @version FooV1 begin
           a::String
           b::Int
       end
F
julia> FooV1(; a="a", b=1)
FooV1: (a = "a", b = 1)

julia> foo = FooV1(; a="a", b=1)
FooV1: (a = "a", b = 1)

julia> @set foo.a="aa"
ERROR: MethodError: no method matching FooV1(::String, ::Int64)

Closest candidates are:
  FooV1(::Any)
   @ Main ~/.julia/packages/Legolas/nhYG7/src/schemas.jl:605

Stacktrace:
 [1] setproperties_object(obj::FooV1, patch::NamedTuple{(:a,), Tuple{String}})
   @ ConstructionBase ~/.julia/packages/ConstructionBase/pAlst/src/ConstructionBase.jl:212
 [2] setproperties(obj::FooV1, patch::NamedTuple{(:a,), Tuple{String}})
   @ ConstructionBase ~/.julia/packages/ConstructionBase/pAlst/src/ConstructionBase.jl:126
 [3] set(obj::FooV1, l::PropertyLens{:a}, val::String)
   @ Accessors ~/.julia/packages/Accessors/4DnIb/src/optics.jl:375
 [4] top-level scope
   @ REPL[8]:1

We could fix this by defining a positional argumetn constructor (namedtuples have them!), or appropriate methods for setproperties_object(::AbstractRecord, ...).

kleinschmidt commented 1 year ago

For integration with ConstructionBase, we basically have two options.

First, we could define (either per-schema version or for all <:AbstractRecords) a positional argument constructor, something like

(::Type{T})(args...) where {T<:Legolas.AbstractRecord} = T(NamedTuple{fieldnames(T)}(args))

AFAICT, this won't conflict with the single-NamedTuple constructor except for cases of a schema with a single field that accepts a NamedTuple as input (which now that I'm writing it out, seems like something I could expect folks to actually hit, albeit rarely).

Second, we could overload ConstructionBase.constructorof to do a similar kind of thing, something like

ConstructionBase.constructorof(::Type{T<:AbstractRecord}) where {T} = (args...) -> T(NamedTuple{fieldnames(T)}(args)

I think this is probably better since it completely walls of the constructor definition from the "normal" construction behavior (it's only used in ConstructorBase-derived operations, like Accessors.@set). I don't know what the consequences of returning an anonymous function like this, but I think it's probably fine given that the type T is in signature?

kleinschmidt commented 1 year ago

A bummer, the second option does not work as written with parametric record types...will mess aroudn a bit more there.