fix: Prune dotfiles rather than ignore, make --prune inject default ignores into prune list#413
fix: Prune dotfiles rather than ignore, make --prune inject default ignores into prune list#413effigies wants to merge 9 commits into
--prune inject default ignores into prune list#413Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #413 +/- ##
==========================================
- Coverage 87.37% 87.35% -0.02%
==========================================
Files 65 65
Lines 4777 4794 +17
Branches 781 780 -1
==========================================
+ Hits 4174 4188 +14
- Misses 510 513 +3
Partials 93 93 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@yarikoptic Care to give this branch a try? |
|
Well, this turned into a rabbit hole. Will try again later. |
--prune inject default ignores into prune list
|
PR description updated. I cast the review net a bit wider, since this is a fairly significant change. |
rwblair
left a comment
There was a problem hiding this comment.
I'm waffling on how transparent we should be with what exactly is pruned when the --prune option isn't used or is false. The defaults are sensible so I don't think we'll surprise any one, but my lack of imagination has tied my shoelaces together before.
Should the contents of ignoreDefaults be more visible in help text or documentation?
Do you think a logger.info readout of pruned files would be useful to anyone?
The nomeclature/semantics of --prune feels strange sometimes, but something like --extended-prune feels worse, so I'm fine with keeping it for now.
| if (options.prune) { | ||
| // Opaque BIDS directories are ignored by default but still included to allow lookups, | ||
| // including size estimates and symlink resolution in git trees. | ||
| // Pruning is an extreme measure to reduce memory usage and IO ops and may have unintended consequences. |
There was a problem hiding this comment.
This may need to be added to the help text for the flag.
Initially narrowly scoped to prune
.git, this PR now includes the following changes:.git/or other potentially deep hidden directories (.venv/, for example).--prunenow injects the default ignore list into the prune list, which is a more principled way to decide what gets pruned.--prune,derivatives/is now ignored..bidsignoreand causing--pruneto un-ignore everything,.bidsignoreis now ignored through a carve-out in theBIDSFile.ignoredproperty. As a validator feature and not a BIDS feature, this seems reasonable to me.Open issues (probably for separate PRs):
dataset_description.json, we're not respecting that, but that's already the case. We already don't load the schema based ondataset_description.json, so as long as we load it from--schema, we're still improving..gitnow being pruned instead of ignored, I think it may be time to revisit this and count ignored files for a more accurate description of dataset size.Closes #412.