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

ConstructionBase integration #99

Merged
merged 9 commits into from
Jul 18, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions Project.toml
Original file line number Diff line number Diff line change
@@ -1,25 +1,31 @@
name = "Legolas"
uuid = "741b9549-f6ed-4911-9fbf-4a1c0c97f0cd"
authors = ["Beacon Biosignals, Inc."]
version = "0.5.13"
version = "0.5.14"

[deps]
Arrow = "69666777-d1a9-59fb-9406-91d4454c9d45"
ConstructionBase = "187b0558-2788-49d3-abe0-74a17ed4e7c9"
Tables = "bd369af6-aec1-5ad0-b16a-f7cc5008161c"
UUIDs = "cf7118a7-6976-5b1a-9a39-7adc72f591a4"

[compat]
Accessors = "0.1"
Aqua = "0.6"
Arrow = "2"
Compat = "3.34, 4"
ConstructionBase = "1.5"
DataFrames = "1"
Tables = "1.4"
julia = "1.6"

[extras]
Accessors = "7d9f7c33-5ae7-4f3b-8dc6-eff91059b697"
Aqua = "4c88cf16-eb10-579e-8560-4a9242c79595"
omus marked this conversation as resolved.
Show resolved Hide resolved
Compat = "34da2185-b29b-5c13-b0c7-acf172513d20"
DataFrames = "a93c6f00-e57d-5684-b7b6-d8193f3e46c0"
Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40"
UUIDs = "cf7118a7-6976-5b1a-9a39-7adc72f591a4"

[targets]
test = ["Compat", "DataFrames", "Test", "UUIDs"]
test = ["Accessors", "Aqua", "Compat", "DataFrames", "Test", "UUIDs"]
1 change: 1 addition & 0 deletions src/Legolas.jl
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
module Legolas

using Tables, Arrow, UUIDs
using ConstructionBase: ConstructionBase

const LEGOLAS_SCHEMA_QUALIFIED_METADATA_KEY = "legolas_schema_qualified"

Expand Down
26 changes: 25 additions & 1 deletion src/schemas.jl
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,24 @@ abstract type AbstractRecord <: Tables.AbstractRow end
@inline Tables.columnnames(r::AbstractRecord) = fieldnames(typeof(r))
@inline Tables.schema(::AbstractVector{R}) where {R<:AbstractRecord} = Tables.Schema(fieldnames(R), fieldtypes(R))

# we need a bit of extra work to integrate with ConstructionBase for pre-1.7 due
# to the overload of `propertynames(::Tables.AbstractRow)`
#
# we _could_ overload `check_properties_are_fields` but that's no part of the
# public API so this is safer
@static if VERSION < v"1.7"
ConstructionBase.getproperties(r::AbstractRecord) = NamedTuple(r)
# largely copy-paste from ConstructionBase.setproperties_object:
omus marked this conversation as resolved.
Show resolved Hide resolved
# https://github.com/JuliaObjects/ConstructionBase.jl/blob/cd24e541fd90ab54d2ee12ddd6ccd229be9a5f1e/src/ConstructionBase.jl#L211-L218
function ConstructionBase.setproperties(r::R, patch::NamedTuple) where {R <: AbstractRecord}
nt = ConstructionBase.getproperties(r)
nt_new = merge(nt, patch)
ConstructionBase.check_patch_properties_exist(nt_new, nt, r, patch)
args = Tuple(nt_new) # old julia inference prefers if we wrap in Tuple
return ConstructionBase.constructorof(R)(args...)
Comment on lines +357 to +361
Copy link
Member

Choose a reason for hiding this comment

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

At first I was curious about improving performance as this converts to a NamedTuple first. As this is also what Tables.rowmerge already does this seems perfectly fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, I was starting to go down that rabbit hole and eventually decided this was probably good enough...

end
end

"""
Legolas.schema_version_from_record(record::Legolas.AbstractRecord)

Expand Down Expand Up @@ -607,7 +625,13 @@ function _generate_record_type_definitions(schema_version::SchemaVersion, record
# generate `inner_constructor_definitions` and `outer_constructor_definitions`
R = record_type_symbol
kwargs_from_row = [Expr(:kw, n, :(get(row, $(Base.Meta.quot(n)), missing))) for n in keys(record_fields)]
outer_constructor_definitions = :($R(row) = $R(; $(kwargs_from_row...)))
outer_constructor_definitions = quote
$R(row) = $R(; $(kwargs_from_row...))
function $ConstructionBase.constructorof(::Type{<:$R})
nt = NamedTuple{$((:($k) for k in keys(record_fields))..., )}
(args...) -> $R(nt(args))
end
end
if isempty(type_param_defs)
inner_constructor_definitions = quote
function $R(; $(field_kwargs...))
Expand Down
42 changes: 42 additions & 0 deletions test/runtests.jl
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
using Compat: current_exceptions
using Legolas, Test, DataFrames, Arrow, UUIDs
using Legolas: SchemaVersion, @schema, @version, SchemaVersionDeclarationError, DeclaredFieldInfo
using Accessors
using Aqua

@test_throws SchemaVersionDeclarationError("no prior `@schema` declaration found in current module") @version(TestV1, begin x end)

Expand All @@ -9,6 +11,12 @@ module Tour
include(joinpath(dirname(@__DIR__), "examples", "tour.jl"))
end

@testset "aqua" begin
# picks up ambiguities in dependencies, and one hit for unbound type
# parameters for `accepted_field_type(::SchemaVersion, ::Type{Union{Missing,T}})`
Aqua.test_all(Legolas; ambiguities=false, unbound_args=false)
end

@testset "Legolas.lift" begin
@test ismissing(Legolas.lift(sin, nothing))
@test ismissing(Legolas.lift(sin, missing))
Expand Down Expand Up @@ -185,6 +193,7 @@ end
module Namespace91
using Test
using Legolas: @schema, @version, tobuffer, read
using Accessors: @set

# Define something else with the name Legolas
struct Legolas end
Expand All @@ -203,6 +212,10 @@ module Namespace91
@test A91V1(; a=1) == A91V1(; a=1)
@test isequal(A91V1(; a=1), A91V1(; a=1))
@test read(tobuffer([A91V1(; a=1)], A91V1SchemaVersion())).a == [1]

a = A91V1(; a=1)
a2 = @set a.a = 2
@test a2.a == 2
Comment on lines +215 to +218
Copy link
Member

Choose a reason for hiding this comment

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

Why are these tests needed? Seems like the later testset covers this. At the very least this should be made into a separate testset inside this module

Copy link
Member Author

Choose a reason for hiding this comment

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

just to make sure that the ConstructionBase name is interpolated correctly as well, since we generate code with $ConstructionBase. in teh macro

Copy link
Member

Choose a reason for hiding this comment

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

Okay, would have been nice to have this as a separate testcase inside the submodule as mentioned. I'll leave this unresolved

end
end # module

Expand Down Expand Up @@ -686,3 +699,32 @@ end
@test_throws e FieldErrorV3(; a="foo bar")
end
end

#####
##### ConstructionBase/Accessors integration
#####

@testset "ConstructionBase/Accessors integration" begin
p = ParentV1(; x=[1,2,3], y="hello")
p2 = @set p.y = "okay"
@test p2 isa ParentV1
@test p2.y == "okay"
@test p2 !== p
@test p2.x === p.x
kleinschmidt marked this conversation as resolved.
Show resolved Hide resolved
@test p2.y != p.y

e = ArgumentError("Invalid value set for field `y`, expected AbstractString, got a value of type Int64 (1)")
@test_throws e @set p.y = 1

n = NestedV1(; gc=GrandchildV1(; x=[1,2], y="hello", z=nothing, a=1), k=:okay)
n2 = @set n.gc.z = "yay!"
@test n2.gc.z == "yay!"
@test n2.gc != n.gc
# parametric on :k
@test typeof(n2) == typeof(n)
@test n2.k === n.k

n3 = @set n.k = missing
@test n3 isa NestedV1{Missing}
@test ismissing(n3.k)
end
Loading