-
Notifications
You must be signed in to change notification settings - Fork 5
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
refactor to latest Legolas version + onda.signal v2 #133
Conversation
eda753d
to
d71ebff
Compare
Co-authored-by: Eric Hanson <[email protected]>
Co-authored-by: Eric Hanson <[email protected]>
07b0b3b
to
bf20faa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
Legolas.accepted_field_type(::SamplesInfoV2SchemaVersion, ::Type{String}) = AbstractString | ||
Legolas.accepted_field_type(::SamplesInfoV2SchemaVersion, ::Type{Vector{String}}) = AbstractVector{<:AbstractString} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should understand this but in the likely case I don't, does this mean:
- If a table has been serialized with some other
AbstractString
for e.g.sample_unit
, it can be deserialized OK? - do we need
sample_unit::String = string(sample_unit)
in the@version
invocation? orString(sample_unit)
? If not, why not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a table has been serialized with some other AbstractString for e.g. sample_unit, it can be deserialized OK?
yup
do we need sample_unit::String = string(sample_unit) in the
@version
invocation? or String(sample_unit)? If not, why not?
Declaring a required field as f::T
is Legolas shorthand for f::T = f
, which is Julia shorthand for f::T = convert(T, f)
. This matches the previous behavior on main
which had the convert
call explicitly (and unnecessarily)
We could add = string(sample_unit)
to auto-stringify non-string-convertable inputs, but in this case, I'd rather callers just do that themselves if that's what they want
@row
/Row
) and improve API Legolas.jl#54) (and will close write_annotations/write_signals errors when given empty tables #82, since those functions are being deprecated in the switch to Legolas v0.5)onda.signal
#119Tests pass locally, but will require beacon-biosignals/TimeSpans.jl#51 to pass in CI.
Otherwise, I still have some documentation/deprecation work to do here; will be in draft mode until that's done, but would very much welcome review. It'll be important Beacon-timeline-wise to get this merged/tagged by EOW.
Note that changes that were breakages in Legolas will be hard to deprecate here, but I'll try to provide decent upgrade paths where possible. However, similarly to beacon-biosignals/Legolas.jl#54: