-
Notifications
You must be signed in to change notification settings - Fork 26
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
Updating BIDS validator and schema to contemporary upstream equivalent #1050
Merged
Merged
Changes from all commits
Commits
Show all changes
30 commits
Select commit
Hold shift + click to select a range
a13418a
Upstream validation features up to and including:
TheChymera 46988a3
Schema update
TheChymera 8405a99
Reverting to DANDI fork of bids-examples
TheChymera fc5a720
Docstring fix
TheChymera 4baf689
Record BIDS schema version, analogous to:
TheChymera 34a2af2
Logging to appdirs when BIDS validation is called via DANDI wrapper
TheChymera a67c91e
Moved external modules to dandi/support/*/
TheChymera cff8c89
formatting changes to stay in sync with upstream BIDS
TheChymera 2ed8929
Corrected help text
TheChymera 73a2061
Updated report docstring in wrapper
TheChymera c367e05
verify report file creation
TheChymera b514d8a
Moved bash usage example to CLI function
TheChymera 24038e8
Formatting
TheChymera 2e53449
Better windows support
TheChymera d4640bf
Improved windows support
TheChymera d35e5f2
Dropped debugging print line
TheChymera cd0b36c
Nicer conditional for recursion
TheChymera a444368
Removed deprecated parameter
TheChymera 4d4a7cf
Reinstated option for report path specification
TheChymera 3bcc542
code style improvement
TheChymera 140a7cf
style improvements
TheChymera 335bf62
Merge branch 'bids_update' of github.com:dandi/dandi-cli into bids_up…
TheChymera b582485
Improved windows support
TheChymera 02a6e95
Using pytest's tmp_path
TheChymera 8c1dbcc
Updated library name
TheChymera 41fa4ac
removed unused default values from function wrapped with click
TheChymera 75e8fc3
Simplified directory exclusion logic
TheChymera ae6970a
Merge branch 'bids_update' of github.com:dandi/dandi-cli into bids_up…
TheChymera 3f43905
Typo
TheChymera 468e39c
Do not index top-level files from pseudofile directories
TheChymera File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
11 changes: 0 additions & 11 deletions
11
dandi/support/bids/schemadata/1.7.0+012/rules/associated_data.yaml
This file was deleted.
Oops, something went wrong.
20 changes: 0 additions & 20 deletions
20
dandi/support/bids/schemadata/1.7.0+012/rules/tabular_metadata.yaml
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
0.3.0 |
Oops, something went wrong.
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.
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.
so we are breaking CLI ... why not just to keep
--report
to be an option to provide the path and if not specified, assume that no report writing was requested - print to the screen?Note: I might not be even able to write to current directory (dataset might be owned by someone else).
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.
functionality wasn't broken, help text was just incorrect.
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.
You removed
--report-flag
and made--report
from taking a string (path) into beingis_flag
-- so, it got broken that user no longer would be able to say--report myvalidation.log
. And I think it is worth removing somewhat odd--report-flag
so ok to break but we better "make it right" this time and avoid future breakage.If you really want to be fancy and support also
bool
like behavior, I guess you would need to make itnargs='?'
(if click supports it, @jwodder could help) and then ifnot report
- assign that default path to the log, so user could do both--report
and--report mylog.log
.Alternative -- not bother with "bool" like behavior in CLI, and just make it demand path string.
NB: please do not mark such comments Resolved - let original Author decide if they were resolved or not since it makes it harder for a reviewer to locate prior comments while re-reviewing and see if prior concerns were addressed.