FIX: non-existing path or JSON syntax error for --bids-filter-file should raise on error.

[bpinsard]

Changes proposed in this pull request

Check if provided bids-filter-file path exists if not raise an error.
This should avoid cases when the user provides file filters and the pipeline silently runs on other set of file.

Documentation that should be reviewed

none

[pep8speaks]

Hello @bpinsard! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-06-23 16:24:46 UTC

[mgxd]

LGTM - this should probably be backported to the LTS as well. It should be a relatively clean process if you change the target branch to maint/20.2.x

[mgxd]

maybe a FileNotFoundError would be more helpful here?

Suggested change

	else:
	raise parser.error(f"Path does not exist: <{value}>.")
	raise FileNotFoundError(f"Path does not exist: <{value}>.")

[bpinsard]

I followed what is done elsewhere in the parser:
https://github.com/nipreps/fmriprep/blob/962f0b2e26d522d87ed786f12766001aa65e9dfd/fmriprep/cli/parser.py#L20-L24
but I have no problem changing it.

[mgxd]

sure that works too, though if you are following that example the function should be converted into a partial:

fmriprep/fmriprep/cli/parser.py

Lines 76 to 78 in c707938

	PathExists = partial(_path_exists, parser=parser)
	IsFile = partial(_is_file, parser=parser)
	PositiveInt = partial(_min_one, parser=parser)

[bpinsard]

oh right, my mistake. Would there be any advantage of making it a partial for that specific option?

[mgxd]

i think its necessary to pass in the defined parser

[effigies]

Have we resolved this discussion? Would it be worth adding a test to make sure that it works, or are there tests in the CI that are covering the necessary cases?

[mgxd]

I think the partial call got lost in the latest rebase

[effigies]

Should this target maint/20.2.x?

[mgxd]

@bpinsard I think the CI errors have been fixed, you'll need to rebase off the maint/20.2.x branch

[effigies]

Rebase again?

[effigies]

@bpinsard I added a test and fixed a bug. This LGTM at this point, feel free to merge when tests pass.