[MRG] Raise an error message when src==dest in write_raw_bids

[adam2392]

PR Description

Closes: #867

• Adds an error for src == dest during write_raw_bids
• Changes writing of EDF to use _write_raw_edf, since EDFlib-Python is a supported EDF writer now
• Adds a unit test to track this behavior

The reason we can't accommodate copying files is outlined in the issue.

Merge checklist

Maintainer, please confirm the following before merging:

• All comments resolved
• This is not your own PR
• All CIs are happy
• PR title starts with [MRG]
• whats_new.rst is updated
• PR description includes phrase "closes <#issue-number>"

[agramfort]

@adam2392 let me know when it's good to go from your end

[adam2392]

Ugh million tests to change cuz of write_edf...

[adam2392]

Ugh million tests to change cuz of write_edf...

[sappelhoff]

Can't we solve this without raising an error? For example by:

1. moving the "file removal in case of overwriting" step further below
2. then being able to check whether removal would result in an impossible copyfile call
3. and if that IS the case, do a temp backup copy before removal
4. then use the temp backup copy for the copyfile call
5. finally delete the temp backup copy

[sappelhoff]

I don't think you should switch to _write_edf. We want to just copy the files in order to manipulate the raw files themselves as little as possible :-)

[hoechenberger]

What's the status here?

[sappelhoff]

This PR wants to do:

1 Adds an error for src == dest during write_raw_bids
2 Changes writing of EDF to use _write_raw_edf, since EDFlib-Python is a supported EDF writer now
3 Adds a unit test to track this behavior

I think point 2 should be dropped from this PR and (if at all) be done in a separate PR. I say "if at all", because

I don't think you should switch to _write_edf. We want to just copy the files in order to manipulate the raw files themselves as little as possible :-)

(see #889 (comment) and #889 (comment))

Regarding point 1 and 3: Yes, +1 to do this. Having an explicit (and covered) error for this scenario would be an improvement over the status quo. But that error doesn't need to be a big and advertised thing, and not framed as a "limitation of mne-bids" IMHO (see #889 (comment))

All in all, I think this PR could be fixed with a small couple of lines to

1. detect the copy issue scenario
2. raise a small error that we don't support that
3. add a test to trigger that error

[codecov]

Codecov Report

Merging #889 (81a3f02) into main (213c378) will decrease coverage by 0.02%.
The diff coverage is 90.90%.

@@            Coverage Diff             @@
##             main     #889      +/-   ##
==========================================
- Coverage   95.19%   95.16%   -0.03%     
==========================================
  Files          25       25              
  Lines        3787     3786       -1     
==========================================
- Hits         3605     3603       -2     
- Misses        182      183       +1     

Impacted Files	Coverage Δ
mne_bids/config.py	97.61% <ø> (ø)
mne_bids/path.py	97.67% <ø> (ø)
mne_bids/report/_report.py	85.71% <ø> (ø)
mne_bids/write.py	97.14% <88.88%> (+<0.01%)	⬆️
mne_bids/read.py	96.00% <100.00%> (-0.03%)	⬇️
mne_bids/utils.py	95.74% <100.00%> (ø)
mne_bids/dig.py	98.27% <0.00%> (-0.35%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[sappelhoff]

ready for review, not that I also bumped minimum requirements for mne to 1.0 -- wdyt?

[sappelhoff]

@hoechenberger @adam2392 @agramfort

[hoechenberger]

all good, except for changes to the docstring of BIDSPath, and the type of an exception you're raising

[hoechenberger]

why is this line never hit by tests? should get coverage IMHO

[sappelhoff]

The line is tested somewhat here:

mne-bids/mne_bids/tests/test_write.py

Lines 1858 to 1866 in 213c378

	# proceed with the actual test for EEGLAB data
	write_raw_bids(raw, bids_path, overwrite=False)
	read_raw_bids(bids_path=bids_path)
	with pytest.raises(TypeError, match="unexpected keyword argument 'foo'"):
	read_raw_bids(bids_path=bids_path, extra_params=dict(foo='bar'))
	with pytest.raises(FileExistsError, match="already exists"): # noqa: F821
	write_raw_bids(raw, bids_path, overwrite=False)

The reason it is never hit is because other files (like events.tsv) get written earlier than the neural data file in write_raw_bids, and these raise an identical error:

mne-bids/mne_bids/utils.py

Lines 206 to 207 in 213c378

	raise FileExistsError(f'"{fname}" already exists. '
	'Please set overwrite to True.')

(... but in a different line)

[hoechenberger]

So … then we can just remove this else branch, no?

[sappelhoff]

I'd leave it here. People may manually remove all other files and then this line would reasonably trigger. it's just not covered in our tests, which I find okay (and which I did not introduce here) :)

[hoechenberger]

alright, let's leave it here then!

[hoechenberger]

Thanks, @sappelhoff!