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

[v0.13] Backport to accept Legolas metadata #106

Open
wants to merge 3 commits into
base: release-v0.13
Choose a base branch
from

Conversation

ericphanson
Copy link
Member

This PR is to the release-v0.13 branch (introduced recently for #105) and allows tables written by Onda v0.14 to be read on Onda v0.13. Ideally consumers can just update to Onda v0.14, but that can be tricky when it is a deep dependency (so might require releases of many packages), so this should help ease the transition (since compat won't prevent patch updates, so you only need to update the patch of your current project rather than compat bounds everywhere in the stack-- though we should do that too 😄).

@@ -1,7 +1,7 @@
name = "Onda"
uuid = "e853f5be-6863-11e9-128d-476edb89bfb5"
authors = ["Beacon Biosignals, Inc."]
version = "0.13.9"
version = "0.13.10"
Copy link
Member

Choose a reason for hiding this comment

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

are we sure this won't mess with the YAML parsers ;;;;)))))

Copy link
Member

@jrevels jrevels left a comment

Choose a reason for hiding this comment

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

Hmm...

valid signal/annotation tables in Onda v0.14 are a superset of the tables that are valid in Onda v0.13 IIRC because Onda v0.14 drops the column ordering requirement

So you could have a valid Onda v0.14 table that this PR allows the user to load in v0.13, but then errors when validated (e.g. naive calls to read_annotations or read_signals could break unless you pass validate_schema=false). unfortunately, I don't think this is unlikely since column ordering isn't a concern/guarantee in Legolas land.

Maybe it'd be easy to change e.g. validate_annotation_schema/validate_signal_schema so that column ordering is only "soft" validated, e.g. raises a warning instead of exception if things aren't as expected?

otherwise this LGTM 👍

@kleinschmidt
Copy link
Member

is this still worth doing @ericphanson ?

@ericphanson
Copy link
Member Author

Uhh, I don't totally remember the context but if we still need this then I can try to implement Jarrett's suggestion and proceed with a backport release.

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 this pull request may close these issues.

3 participants