-
Notifications
You must be signed in to change notification settings - Fork 4
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
EVA-3564 - Simplify metadata conversion and validation #55
Conversation
apriltuesday
commented
Sep 10, 2024
•
edited
Loading
edited
- Metadata conversion never fails (unless unable to open the excel file) and always generates a JSON to validate
- Scientific name and Biosample name errors correctly parsed and propagated to report
- Added scientific name / taxonomy ID coherence check to semantic validation
- Refactored and updated tests
/sample/3/bioSampleObject/name | ||
must have required property 'name' | ||
must have required property 'name' | ||
must have required property 'name' |
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.
not sure what's going on here, but this is the output I get from biovalidator...
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.
Looks great
if scientific_name in data: | ||
data[SPECIES] = data[scientific_name] |
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.
Is it worth having a mechanism that convert a excel spreadsheet into more than one BioSample field ?
We could then provide this in the spreadsheet2json_conf.yaml
as
Sample:
header_row: 3
optional:
Scientific Name: [scientificName, species]
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.
Could do, it would only be used for this case though as far as I know - the name field goes outside of characteristics so has to be handled differently. I probably won't add it to this PR but we should keep it in mind.
# Sometimes there are multiple (possibly redundant) errors listed under a single property, | ||
# we only report the first |
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.
We probably should report that to biovalidator.js
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.
Good point, I'll make an issue for them.