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

MRG: Don't create directories when accessing BIDSPath.fpath #1139

Merged
merged 3 commits into from May 28, 2023

Conversation

hoechenberger
Copy link
Member

@hoechenberger hoechenberger commented May 28, 2023

Don't unexpectedly create directories when users don't actually intend to alter the BIDS dataset.

The problem was reported by multiple users on the forum:
https://mne.discourse.group/t/bidspath-creating-new-directory-when-specified-dir-doesnt-exist/6928

And it's also something that has actually bothered me for years.

I simply removed an .mkdir() call, which seemed unnecessary anyway.

Fails on main, passes on this branch:

from pathlib import Path
import tempfile
from mne_bids import BIDSPath, print_dir_tree


with tempfile.TemporaryDirectory() as bids_root:
    bids_root = Path(bids_root)
    bp = BIDSPath(subject='01', datatype='eeg', root=bids_root)
    bp.fpath  # accessing .fpath is required for this regression test
    assert not (bids_root / 'sub-01').exists()

Merge checklist

Maintainer, please confirm the following before merging.
If applicable:

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

@codecov
Copy link

codecov bot commented May 28, 2023

Codecov Report

Merging #1139 (19e751a) into main (96a4532) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1139   +/-   ##
=======================================
  Coverage   95.28%   95.28%           
=======================================
  Files          40       40           
  Lines        8695     8699    +4     
=======================================
+ Hits         8285     8289    +4     
  Misses        410      410           
Impacted Files Coverage Δ
mne_bids/path.py 97.70% <100.00%> (ø)
mne_bids/tests/test_path.py 99.84% <100.00%> (+<0.01%) ⬆️

Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

I agree that it's cleaner to not create directories here and there. I just wonder why we had it in the first place.

@hoechenberger
Copy link
Member Author

I agree that it's cleaner to not create directories here and there. I just wonder why we had it in the first place.

Maybe it was just a leftover from some old refactoring. I was very surprised to see the entire test suite pass after removing it. 😅

@hoechenberger hoechenberger marked this pull request as ready for review May 28, 2023 10:20
@hoechenberger hoechenberger changed the title Don't create directories when accessing BIDSPath.fpath MRG: Don't create directories when accessing BIDSPath.fpath May 28, 2023
doc/whats_new.rst Outdated Show resolved Hide resolved
@hoechenberger hoechenberger merged commit a3bf0b0 into mne-tools:main May 28, 2023
15 checks passed
@hoechenberger hoechenberger deleted the dont-create-dir branch May 28, 2023 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants