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

use EDFAnnotation schema for import/export #55

Closed
wants to merge 9 commits into from

Conversation

kleinschmidt
Copy link
Member

This fixes #54 by enforcing this assumption with a schema (edf.annotation) which extends onda.annotation with one extra field value::String. It also changes the return type of imported EDF annotation to these rows (although the values are identical); while this is technically breaking, it seems pretty harmless.

@kleinschmidt kleinschmidt requested a review from omus July 19, 2022 21:29
EDF file. The only extra field is `value`, which holds the string stored in the
elements of `annotations` in the EDF annotation.
"""
const EDFAnnotation = @row("edf.annotation@1" > "onda.annotation@1",
Copy link
Member

Choose a reason for hiding this comment

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

In following the convention for Plan shouldn't this be?

Suggested change
const EDFAnnotation = @row("edf.annotation@1" > "onda.annotation@1",
const Annotation = @row("ondaedf.annotation@1" > "onda.annotation@1",

Copy link
Member Author

Choose a reason for hiding this comment

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

I think my intention here was that this is an annotation that is derived from an EDF Annotation signal. The fact that it's imported by OndaEDF seems a bit beside the point (unlike the plan which is intrinsically an OndaEDF thing)

@@ -38,6 +38,7 @@ jobs:
path: ~/.julia/artifacts
key: ${{ runner.os }}-test-artifacts-${{ hashFiles('**/Project.toml') }}
restore-keys: ${{ runner.os }}-test-artifacts
- run: julia --project -e 'using Pkg; Pkg.develop([PackageSpec(; path="OndaEDFSchemas.jl")])'
Copy link
Member

Choose a reason for hiding this comment

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

Should only be needed until OndaEDFSchemas.jl is registered. My personal preference would be to have this work split into two PRs. Also, I do find using a committed Manifest.toml is a cleaner way to do that but mainly that's only useful while developing the PR

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it is registered at this point. Without this the registered version is picked up, which means that the OndaEDF CI will not pick up the OndaEDFSchemas changes in a PR.

@kleinschmidt
Copy link
Member Author

we're going to do this as part of the Legolas 0.5 upgrade #60

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.

export assumes value field is present in the annotations
2 participants