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

fix(cli): Allow blacklisting any modality from schema.rules.modalities #2176

Merged
merged 2 commits into from
Oct 28, 2024

Conversation

effigies
Copy link
Collaborator

Before:

  --blacklistModalities  <modalities...>  - Array of modalities to error on if detected.                                    (Default: [], Values: "MRI", "PET", "MEG", "EEG", "iEEG",
                                                                                                                            "Microscopy", "NIRS", "MRS")

After:

  --blacklistModalities  <modalities...>  - Array of modalities to error on if detected.                                    (Default: [], Values: "mri", "eeg", "ieeg", "meg", "beh",
                                                                                                                            "pet", "micr", "motion", "nirs", "mrs")

We previously used "Microscopy", but we looked up the value from schema.rules.modalities, so that would fail. We could map the display names, but I don't think it will hurt anybody to use micr. We also weren't supporting motion.

This won't help with patched schemas introducing new modalities, but blacklisting a modality introduced by a patch seems like an extremely niche use case. It would be nice if we could get the suggested values without actually limiting, as there's no harm in blacklisting something that doesn't exist. But the main use case would be OpenNeuro blacklisting a modality, and we skip over argument parsing and call validate() directly.

This PR adds a check so that tools calling via the API will warn, not crash, if passing an unknown modality.

@effigies effigies force-pushed the fix/blacklist-inputs branch from 75a44e8 to 649ed06 Compare October 25, 2024 18:03
Copy link

codecov bot commented Oct 25, 2024

Codecov Report

Attention: Patch coverage is 77.77778% with 2 lines in your changes missing coverage. Please review.

Project coverage is 87.48%. Comparing base (f40d4de) to head (525163b).
Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
bids-validator/src/validators/bids.ts 71.42% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    bids-standard/legacy-validator#2176      +/-   ##
==========================================
+ Coverage   85.72%   87.48%   +1.75%     
==========================================
  Files          91      133      +42     
  Lines        3784     7055    +3271     
  Branches     1221     1671     +450     
==========================================
+ Hits         3244     6172    +2928     
- Misses        454      788     +334     
- Partials       86       95       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@effigies effigies force-pushed the fix/blacklist-inputs branch from 649ed06 to a664be1 Compare October 25, 2024 18:13
@effigies effigies force-pushed the fix/blacklist-inputs branch from a664be1 to 525163b Compare October 25, 2024 18:28
@effigies effigies requested a review from rwblair October 25, 2024 18:38
@rwblair rwblair merged commit d2c9fbb into bids-standard:master Oct 28, 2024
18 of 19 checks passed
@effigies effigies deleted the fix/blacklist-inputs branch October 28, 2024 17:16
blacklistedDatatypes.set(datatype, modality)
}
} else {
logger.warn(`Attempted to blacklist unknown modality: ${modality}`)
Copy link
Contributor

Choose a reason for hiding this comment

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

FTR, relates to

  • #2182

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants