Support convert on Row

[omus]

MWE of the original issue encountered which this PR addresses:

julia> using UUIDs, TimeSpans, Onda

julia> nt = (; recording=uuid4(), file_path="./file", file_format="lpcm", span=TimeSpan(1, 100), kind="kind", channels=string.('a':'c'), sample_unit="microvolt", sample_resolution_in_unit=1.0, sample_offset_in_unit=0.0, sample_type="uint16", sample_rate=256)
(recording = UUID("a57b8e4b-94a2-43d7-a02a-03c1c7ec5ab8"), file_path = "./file", file_format = "lpcm", span = TimeSpan(00:00:00.000000001, 00:00:00.000000100), kind = "kind", channels = ["a", "b", "c"], sample_unit = "microvolt", sample_resolution_in_unit = 1.0, sample_offset_in_unit = 0.0, sample_type = "uint16", sample_rate = 256)

julia> signals = Onda.Signal[]
Legolas.Row{Legolas.Schema{Symbol("onda.signal"), 1}, F} where F[]

julia> push!(signals, nt)
ERROR: MethodError: Cannot `convert` an object of type
  NamedTuple{(:recording, :file_path, :file_format, :span, :kind, :channels, :sample_unit, :sample_resolution_in_unit, :sample_offset_in_unit, :sample_type, :sample_rate), Tuple{UUID, String, String, TimeSpan, String, Vector{String}, String, Float64, Float64, String, Int64}} to an object of type
  Legolas.Row{Legolas.Schema{Symbol("onda.signal"), 1}, F} where F
Closest candidates are:
  convert(::Type{T}, ::T) where T at essentials.jl:205
  (Legolas.Row{Legolas.Schema{Symbol("onda.signal"), 1}, F} where F)(::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Any; custom...) at deprecated.jl:70
  (Legolas.Row{S, F} where F)(::Any...; kwargs...) where S at /Users/omus/.julia/dev/Legolas/src/rows.jl:168
Stacktrace:
 [1] push!(a::Vector{Legolas.Row{Legolas.Schema{Symbol("onda.signal"), 1}, F} where F}, item::NamedTuple{(:recording, :file_path, :file_format, :span, :kind, :channels, :sample_unit, :sample_resolution_in_unit, :sample_offset_in_unit, :sample_type, :sample_rate), Tuple{UUID, String, String, TimeSpan, String, Vector{String}, String, Float64, Float64, String, Int64}})
   @ Base ./array.jl:932
 [2] top-level scope
   @ REPL[4]:1

[jrevels]

Suggested change

	Base.convert(::Type{Row{S}}, fields) where {S} = Row(S(), fields)
	Base.convert(::Type{Row{S}}, fields::Row) where {S} = Row(S(), fields) # Dispatch ambiguity fix
	Base.convert(::Type{Row{S}}, fields) where {S} = Row(S(), fields)
	Base.convert(::Type{Row{S}}, fields::Row) where {S} = Row(S(), fields) # Dispatch ambiguity fix
	Base.convert(::Type{Row{S}}, fields::Row{S}) where {S} = fields

this might be overly defensive, but I have too many battle scars tracking down performance issues to implicit no-op conversions that accidentally dispatch to a copy-triggering method

[jrevels]

Actually this brings up a philosophical issue; Row construction is not necessarily idempotent for any given schema.

So there's a choice here of whether we want to actually make conversion semantics the same as construction semantics

This makes me think we shouldn't merge this; the caller should be required to make these kinds of choices/drive this behavior explicitly.

In other words, I think in the example in the OP the caller should explicitly push!(signals, Signal(nt)); depending on the situation the error that gets raised otherwise is actually "a feature" and not just an inconvenience depending on the caller's actual intentions, so we should continue to force them to express those intentions explicitly

[omus]

I think in the example in the OP the caller should explicitly push!(signals, Signal(nt)); depending on the situation the error that gets raised otherwise is actually "a feature" and not just an inconvenience depending on the caller's actual intentions, so we should continue to force them to express those intentions explicitly

In that case I'd advocate for defining convert and having it raise an error message explaining that intent

[jrevels]

Good idea, I'd support that

[jrevels]

Do we find it weird to support convert(::Type{Row{S}}, ...) but not convert(::Type{<:Row{S}}, ...)

[omus]

I agree. I think with the current inner constructor this could be tricky. I'll give this some thought

[ericphanson]

Cool! I wonder about the semantics of the transforms. E.g. with weird/wild transformations like

Legolas.jl/examples/tour.jl

Line 25 in c7e0b13

b::String = string(a, b, c),

should they trigger here? Or if we are converting, should we expect the thing we are converting already had those transformations applied but just lost it's type tag somewhere?

I think it could be useful to add something to the tour to show off this behavior so at least it's documented which way it works.

[jrevels]

I think I'm against adding this ref https://github.com/beacon-biosignals/Legolas.jl/pull/27/files#r739363377

[jrevels]

closing as stale, will open an issue for the action item here: #27 (comment)