bids-standard / legacy-validator

Validator for the Brain Imaging Data Structure
https://bids-standard.github.io/bids-validator/
MIT License
185 stars 111 forks source link

Pass value of ignore test to creation of filetree object. #2152

Closed rwblair closed 3 weeks ago

rwblair commented 3 weeks ago

Certain directories get contexts during calls to walk, ignore test needs to be run and set before then. Noticed certain directories in sourcedata of ds004720 triggering NOT_INCLUDED errors.

I still need to write tests for this.

Fixes https://github.com/bids-standard/bids-validator/issues/2127.

codecov[bot] commented 3 weeks ago

Codecov Report

Attention: Patch coverage is 69.69697% with 10 lines in your changes missing coverage. Please review.

Project coverage is 87.66%. Comparing base (ae54efa) to head (ae52740). Report is 12 commits behind head on master.

Files with missing lines Patch % Lines
bids-validator/src/files/browser.ts 40.00% 3 Missing :warning:
bids-validator/src/files/deno.ts 78.57% 3 Missing :warning:
bids-validator/src/validators/json.ts 0.00% 2 Missing :warning:
bids-validator/src/schema/applyRules.ts 0.00% 1 Missing :warning:
bids-validator/src/schema/context.ts 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #2152 +/- ## ========================================== + Coverage 85.73% 87.66% +1.93% ========================================== Files 91 133 +42 Lines 3785 7030 +3245 Branches 1220 1666 +446 ========================================== + Hits 3245 6163 +2918 - Misses 454 772 +318 - Partials 86 95 +9 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

effigies commented 3 weeks ago

Should we just prune sourcedata/ and code/ in the same way we do derivatives, except unconditionally?

effigies commented 3 weeks ago

Another approach could be to adapt:

https://github.com/bids-standard/bids-validator/blob/ae54efa0feb080f7ee42494e12c723cd234acc65/bids-validator/src/schema/walk.ts#L42-L44

To:

-    if (dsContext.isPseudoFile(dir)) {
+    if (dsContext.isPseudoFile(dir) && !ignore.test(dir.path)) {
       yield new BIDSContext(pseudoFile(dir), dsContext)
     } else {

We would need to expose the ignore object somewhere outside the BIDSFiles to make this work, but maybe that's better than the current approach, where we inject a singleton and then update it before we lose its reference.

rwblair commented 3 weeks ago

Yeah I'm not a fan of how ignore is being managed right now. I'll play with loading bidsignore earlier and storing in the dataset context like other configuration options.

Should a bidsignore file in a root dataset apply to nested datasets?

effigies commented 3 weeks ago

Should a bidsignore file in a root dataset apply to nested datasets?

I don't think so.

rwblair commented 3 weeks ago

This latest commit fails, cli and web, on ds001583, which has its .bidsignore as a directory instead of a file. Guess I'll make it survive any error but log/warn on non NOENT errors. ds001583 is only dataset I've found with a .bidsignore as a directory.