Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[ENH] Added the specification for using HED libraries in BIDS #1106
[ENH] Added the specification for using HED libraries in BIDS #1106
Changes from 12 commits
6ce58dc
976559a
6555ad5
dc723e4
d52c011
8039ec9
ccf749b
ce26293
a0a7b36
37e9b8f
42708ef
f26bcd1
cfea5f7
5310be6
a49df64
55d79a6
5c7045d
6101459
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
might be beneficial to think about what validation we can build into the schema here apart from "string". E.g., checking for permissible versioning formats, occurrence of
:
and_
characters, etc.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.
That is a good idea if someone knows how to do it that would be great.
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.
Try adding this behemoth regex to
src/schema/objects/formats.yaml
(the first two groups are for the library nickname and library name, and the rest was adapted from https://semver.org/#is-there-a-suggested-regular-expression-regex-to-check-a-semver-string):^(?:[a-zA-Z]+:)?(?:[a-zA-Z]+_)?(?:0|[1-9]\d*)\.(?:0|[1-9]\d*)\.(?:0|[1-9]\d*)(?:-(?:(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\+(?:[0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?$
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.
thanks @happy5214 what's your opinion on adding this regex for validation? Will it cause more pain than it's worth, or should we do it?
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 was pinged, but I don't know if the question was directed toward me or if you were just thanking me. The tail end of the regex could be simplified to the following if we were to ban the use of pre-release schema versions:
^(?:[a-zA-Z]+:)?(?:[a-zA-Z]+_)?(?:0|[1-9]\d*)\.(?:0|[1-9]\d*)\.(?:0|[1-9]\d*)$
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.
both to thank you and to get your opinion, because you called it a "behemoth" and I wasn't sure whether that means you advise against adding this. :-)
Meanwhile this discussion progresses in the main thread