Follow symlinks recursively #56
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The current implementation did not follow symlink directories, only files. The built-in flag 'recurse_symlinks' was not introduced until py3.13. This commit adds a method for safely recursing directories for local paths (assumes we don't have to worry about symlinks for cloudpaths) Wil need to add some proper tests, but interactive testing suggests this fix works. Eventually, this method can be dropped in favor of path.rglob(*, recurse_symlinks=True) when only py3.13+ is supported.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #56 +/- ##
==========================================
+ Coverage 95.07% 95.15% +0.07%
==========================================
Files 7 7
Lines 487 495 +8
==========================================
+ Hits 463 471 +8
Misses 24 24 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
kaitj
force-pushed
the
maint/rglob
branch
4 times, most recently
from
3380af7 to
f411c9c
Compare
nx10
reviewed
Jun 11, 2025
| @pytest.fixture | ||
| def symlink_dataset(tmp_path: Path, sub: str = "sub-001", ses: str = "ses-001") -> Path: | ||
| data = tmp_path / "data" | ||
| data.mkdir() |
There was a problem hiding this comment.
Does this need to be cleaned up?
There was a problem hiding this comment.
When you say "cleaned up" do you mean the code for the fixture or like the directory itself?
nx10
reviewed
Jun 11, 2025
| @@ -400,7 +402,14 @@ def _index_bids_subject_dir( | |||
| _, subject = path.name.split("-", maxsplit=1) | |||
|
|
|||
| records = [] | |||
| for p in path.rglob("sub-*"): | |||
| if isinstance(path, CloudPath): | |||
There was a problem hiding this comment.
This check is added in 1f77700.
I did need to have separate rglob calls for ClouthPath and py3.13+ - the arguments were a little bit different.
- Use built-in method for ClouthPath's since `glob.glob` does not properly recurse through sub-directories. If it is a local path, just fall back to `glob.glob`. Alternatively, could just check for Python version and only run this if <py3.13. - The built-in rglob methods have slightly different parameters so need different calls for them.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #55
The
path.globthat was used didn't follow directories that were symlinked.In py3.13,From what I was able to figure out, this was only needed when indexing a subject directory. I also added a (I admit messy) pytest fixture for testing indexing with symlinks.recurse_symlinksargument was introduced, but this is still an issue in py3.11 and py3.12. This introduces a method that performs similarly to py3.13 to recurse symlinks as well viaos.walk.I wonder if there is a better fix / way to do this.Turns out yes...the changes stem from trying to unifypathlib.path.globwithglob.glob(see python/cpython#117589). For now, this is falling back onglob.globwith a recursive pattern to support <py3.13.@nx10 - mind taking a look? My brain is doing circles trying to sort out this symlinking stuff.