-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add 'exclude' field #35
Conversation
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.
Thanks @e-kenneally! Left a few minor comments. Can you also add a test_bids2table_exclude
in our tests?
source = Crawler( | ||
root=root, | ||
include=["sub-*"], # find subject dirs | ||
skip=["sub-*"], # but don't crawl into subject dirs | ||
skip=["sub-*"] + exclude, # but don't crawl into subject dirs |
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.
Probably we should also pass exclude
to the Crawler
's own exclude
arg? But this should only matter in the odd case that someone tries to exclude a particular subject directory (which to be fair I'm not sure why anyone would do).
Co-authored-by: Connor Lane <[email protected]>
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.
Looks good, good fix to add exclusions to the second-level extract_bids_subdir
. (For reference, indexing in two phases like this is easier to parallelize.)
Left a few minor suggestions. Once you run the formatting should be good to go.
Co-authored-by: Connor Lane <[email protected]>
Co-authored-by: Connor Lane <[email protected]>
Co-authored-by: Connor Lane <[email protected]>
Fixes issue #20
I added exclude flag for the command line which just feeds into the pre-existing 'skip' field at runtime. Tested locally.