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

walk and walk-sync has problems with absolute paths in certain conditions #1102

Closed
felixsanz opened this issue Aug 5, 2021 · 5 comments
Closed
Labels
bug Something isn't working

Comments

@felixsanz
Copy link

Describe the bug A clear and concise description of what the bug is.

At least walk and walk-sync (don't know other functions), has some weird issue.

Let's say we have a folder /home/username/articles/ with a few .md files inside.

And let's create a wtf.ts file in that folder with this content:

import * as path from 'https://deno.land/std@0.103.0/path/mod.ts'
import { walkSync } from 'https://deno.land/std@0.103.0/fs/mod.ts'

console.log([...walkSync('.', { match: [path.globToRegExp('*.md')] })]) // WORK
console.log([...walkSync('.', { match: [path.globToRegExp('**/*.md')] })]) // WORK

console.log([...walkSync('/home/username/articles', { match: [path.globToRegExp('*.md')] })]) // DOESN'T WORK
console.log([...walkSync('/home/username/articles', { match: [path.globToRegExp('**/*.md')] })]) // WORK

.md files should be listed in all cases, but when using *.md and an absolute path, it doesn't list anything. Either use relative path, or I change glob pattern to **/*.md, which I don't want because it also would walk any subdirectory.

For reference:

> ls /home/username/articles
cookies.md  legal.md  index.md    mit.md    wtf.ts

To Reproduce Steps to reproduce the behavior:

See above

Expected behavior A clear and concise description of what you expected to
happen.

Should list files

Desktop (please complete the following information):

  • OS: [e.g. Ubuntu 20.04, MacOS 11] Linux
  • Version
deno 1.12.1 (release, x86_64-unknown-linux-gnu)
v8 9.2.230.14
typescript 4.3.5

Additional context Add any other context about the problem here.

@felixsanz felixsanz added bug Something isn't working needs triage labels Aug 5, 2021
@ericls
Copy link

ericls commented Aug 5, 2021

As discussed in discord, intuitively I think we should only use the relative part of the file path (ie: path.relative(walkRoot, currentFilePath)) to match against the regex. But need to check other implementations to make sure we are not breaking any conventions and promises.

@getspooky
Copy link
Contributor

getspooky commented Aug 18, 2021

Hi @felixsanz

The easiest fix is to use [/\.md/] instead of path.globToRegExp for example

console.log([...walkSync("/home/username/articles",{ match: [/\.md/] })]); // WORK
console.log([...walkSync('/home/username/articles', { match: [path.globToRegExp('*.md')] })]) // DOESN'T WORK

The issue is related to path.globToRegExp by default it add [^/] in pattern so the final result would be something like /^[^/]*\.md\/*$/. It will return null because the path include /.

@felixsanz
Copy link
Author

felixsanz commented Aug 18, 2021

so it's a bug or a feature?

@felixsanz felixsanz closed this as not planned Won't fix, can't repro, duplicate, stale Jan 24, 2024
@phaux
Copy link

phaux commented May 16, 2024

As discussed in discord, intuitively I think we should only use the relative part of the file path (ie: path.relative(walkRoot, currentFilePath)) to match against the regex.

I agree. I think it makes sense if walk("path", { skip: globToRegExp("glob") }) behaves more like gitignore containing "glob".

@phaux phaux mentioned this issue Jul 2, 2024
6 tasks
@kt3k kt3k reopened this Jul 3, 2024
@kt3k
Copy link
Member

kt3k commented Jul 3, 2024

Looked into fs APIs in more details. This scenario is covered nicely by expandGlob(Sync) which implements higher level look-up operation using glob patterns.

import { expandGlobSync } from "@std/path";

console.log([...expandGlobSync("*.md", { root: "/home/username/articles" })]);
console.log([...expandGlobSync("**/*.md", { root: "/home/username/articles" })]);
console.log([...expandGlobSync("**/*", { root: "/home/username/articles", exclude: ["*.md"] })]);

Please use this pattern if you want to look up or skip using globs.

I now don't think it's good idea to change walk to match patterns with relative paths because it would introduce a lot of path.relative calls which will cause perf degradation and also because it's already covered by expandGlob

@kt3k kt3k closed this as not planned Won't fix, can't repro, duplicate, stale Jul 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

6 participants