fix(fs): walk does not apply exts filter to directories#7167
Conversation
walk and walkSync's include() check ran the exts filter against the directory name when deciding whether to yield a directory entry. Directories generally don't have file extensions, so any `exts` option effectively dropped every directory from the output, even when includeDirs:true was set (the default). Pass undefined for exts in the includeDirs branch so the filter only applies to file entries. The match and skip regex filters still apply to directories. Symlink emission (the !followSymlinks branch) is unchanged because a symlink's name still has an extension just like the file or directory it points to. Existing 'walk() accepts ext option as strings' tests are updated to include the root directory in the expected entries (default includeDirs:true). Two new tests pin the includeDirs:false case so the intent is explicit. Fixes denoland#6736
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7167 +/- ##
=======================================
Coverage 94.57% 94.57%
=======================================
Files 636 636
Lines 52142 52142
Branches 9401 9401
=======================================
Hits 49315 49315
Misses 2249 2249
Partials 578 578 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
bartlomieju
left a comment
There was a problem hiding this comment.
Nice, focused fix — I traced the recursion and confirmed it applies to nested directories too (each subdir becomes the root of a recursive walk() call), and the ext/regExp tests pass on the branch. Three points below; two are inline.
exts JSDoc is now slightly inaccurate (fs/walk.ts:71-77). The doc says "entries without the file extension specified by this option are excluded." That's no longer true for directories. Worth a one-line note like "Directory entries are not subject to this filter" so the docs match the new behavior.
| // Directories don't have file extensions, so the `exts` filter must not be | ||
| // applied when deciding whether to yield a directory entry. The `match` | ||
| // and `skip` regex filters still apply. See #6736. | ||
| if (includeDirs && include(root, undefined, match, skip)) { |
There was a problem hiding this comment.
The same class of bug still exists for symlinks-to-directories in the !followSymlinks branch below (fs/walk.ts:488 and :920): include(path, exts, match, skip) still applies exts to the symlink entry. A symlink pointing at a directory has no extension, so includeSymlinks: true + exts will silently drop it — exactly the issue you're fixing here.
The PR description's justification ("a symlink's name still has an extension just like the target") isn't accurate for symlinks-to-directories. Either handle it consistently here, or — if you'd rather defer it — soften that line in the description so it doesn't read as verified-correct.
|
|
||
| Deno.test("walkSync() accepts ext option as strings (excluding period prefix)", () => | ||
| assertWalkSyncPaths(testdataDir, "ext", [".", "y.rs", "x.ts"], { | ||
| exts: [".rs", ".ts"], |
There was a problem hiding this comment.
Pre-existing copy-paste error (optional fix-along): this (excluding period prefix) test uses exts: [".rs", ".ts"] (with leading periods), which doesn't match its own name. Its async sibling above correctly uses ["rs", "ts"]. Since you're already editing this line, cheap to fix.
Fixes #6736.
walkandwalkSyncran theextsfilter against the directory name ininclude()when deciding whether to yield a directory entry. Directories generally don't have file extensions, so anyextsoption effectively dropped every directory from the output, even withincludeDirs:true(the default).Repro before the patch:
After:
The fix passes
undefinedforextsin theincludeDirsbranch of bothwalkandwalkSync. Thematchandskipregex filters still apply to directories. The!followSymlinkssymlink emission branch is unchanged because a symlink's name still has an extension just like the target.Tests:
walk() / walkSync() accepts ext option as stringstests updated to include the root directory in the expected entries (the defaultincludeDirs:truenow actually yields it).includeDirs:falsecase so the intent stays explicit.All ext-related tests pass. The four pre-existing
* regExpsfailures infs/walk_test.tsreproduce onorigin/main(verified by stashing the patch and re-running) so they're unrelated.