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

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

Merged
merged 9 commits into from
Oct 9, 2024

Conversation

rwblair
Copy link
Member

@rwblair rwblair commented Oct 8, 2024

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.

Copy link

codecov bot commented Oct 8, 2024

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 ⚠️
bids-validator/src/files/deno.ts 78.57% 3 Missing ⚠️
bids-validator/src/validators/json.ts 0.00% 2 Missing ⚠️
bids-validator/src/schema/applyRules.ts 0.00% 1 Missing ⚠️
bids-validator/src/schema/context.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            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     

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

@effigies
Copy link
Collaborator

effigies commented Oct 8, 2024

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

@@ -119,7 +119,7 @@ async function _readFileTree(
): Promise<FileTree> {
await requestReadPermission()
const name = basename(relativePath)
const tree = new FileTree(relativePath, name, parent)
const tree = new FileTree(relativePath, name, parent, ignore.test(join(rootPath, relativePath)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure this will be sufficient, since we could recurse into a directory before reading .bidsignore from the first directory. I think we might instead need to make FileTree do an active lookup into an ignore object like BIDSFileDeno does above.

@effigies
Copy link
Collaborator

effigies commented Oct 8, 2024

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
Copy link
Member Author

rwblair commented Oct 8, 2024

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
Copy link
Collaborator

effigies commented Oct 8, 2024

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

I don't think so.

…lly load library. Maybe better off just attempting text call and seeing if it errors out.
…nditionally load library. Maybe better off just attempting text call and seeing if it errors out."

This reverts commit c02e1f0.
@rwblair
Copy link
Member Author

rwblair commented Oct 8, 2024

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.

@rwblair rwblair merged commit 5991dbb into bids-standard:master Oct 9, 2024
18 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants