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

Allow datasets to specify HED versions or individual files as validation schema #1015

Merged
merged 14 commits into from
Aug 4, 2020

Conversation

happy5214
Copy link
Collaborator

@happy5214 happy5214 commented Jul 15, 2020

closes #942

This implements the changes proposed in bids-standard/bids-specification#527 to allow datasets to specify their HED schema version, or a specific schema file in sourcedata/, to run HED validation against. I have tested both ways with a sample dataset.

@happy5214 happy5214 changed the title Allow datasets to specify HED versions or individual files Allow datasets to specify HED versions or individual files as validation schema Jul 15, 2020
Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

Thanks @happy5214. I'll leave a full review of this to @rwblair ... and we should also probably wait with merging this until the PR bids-standard/bids-specification#527 is merged.

But I'll leave a few comments:

edit: tagging @VisLab for attention

- Add warning issue for missing HEDVersion field.
- Add field to JSON schema.
@happy5214
Copy link
Collaborator Author

happy5214 commented Jul 27, 2020

Thanks @happy5214. I'll leave a full review of this to @rwblair ... and we should also probably wait with merging this until the PR bids-standard/bids-specification#527 is merged.

But I'll leave a few comments:

edit: tagging @VisLab for attention

The issues listed have been addressed (or will be after bids-standard/bids-examples#191 is merged). I left in the local schema code for now, but I can remove it if anyone has a strong opinion on that.

@happy5214
Copy link
Collaborator Author

Is this waiting on the example dataset? That will probably take a while to finish, so perhaps we should merge this first.

@sappelhoff
Copy link
Member

It'd be cool to give @rwblair some more time to review this - and I think bids-standard/bids-specification#527 should be merged first once discussion is done over there.

}

// run HED validator
return hedValidator.buildSchema(schemaDefinition).then(hedSchema => {
Copy link
Member

Choose a reason for hiding this comment

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

I'm ok with leaving in the local schema ability, can we add a catch statement on this promise chain? If HEDVersion is not a semantic version and the file doesn't exist it produces an uncaught error. I think generating a new issue here would be good. I figure most end users will have mistyped the semantic version rather than be trying to use their own schema.

I think the catch is also good in case the hed validator ever rejects its promise during build schema for whatever reason: https://github.com/hed-standard/hed-javascript/blob/f9674dfc6a18ec068260559b6d485b6ca61d0298/validators/schema.js#L374

Copy link
Member

Choose a reason for hiding this comment

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

Sorry I didn't realize that I had left this unsubmitted @happy5214 I'm going to go ahead and fix the merge conflict and merge this, and create a new issue for my concern here.

@rwblair rwblair merged commit cb31b11 into bids-standard:master Aug 4, 2020
@happy5214 happy5214 deleted the hed-schema-specification branch December 11, 2020 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need permission to update the HED validator component of BIDS for units
3 participants