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

fix(fs/walk): return symbolic links (#1359) #3455

Closed

Conversation

joehillen
Copy link
Contributor

@joehillen joehillen commented Jun 17, 2023

Symbolic links were not returned. This means that walk() is not listing all the contents of a directory.

Some use cases for this is are:

  • Copying a directory recursively.
  • Finding invalid symbolic links.
  • Modify link targets without resolving them.

Other changes:

  • walk_test.ts: Rename walkArray() to collectPaths() because it is more descriptive.
  • walk_test.ts: Add collectEntries()

closes #1359

@joehillen joehillen requested a review from kt3k as a code owner June 17, 2023 04:02
@kt3k
Copy link
Member

kt3k commented Jun 20, 2023

Looks like reasonable changes to me. Thanks for your suggestions!

Can you take a look into the test failures in fs/expand_glob_test.ts? Can you also run deno fmt to pass the lint check?

@github-actions github-actions bot added the fs label Jul 11, 2023
@joehillen joehillen force-pushed the fix/1359/walk-return-links branch 2 times, most recently from ce4c0fc to 4c23486 Compare July 18, 2023 21:48
@joehillen
Copy link
Contributor Author

@kt3k Should be fixed now. Sorry for the delay. It's been a crazy month.

Symbolic links were not returned. This means that `walk()` is not listing
all the contents of a directory.

Some use cases for this is are:

- Copying a directory recursively.
- Finding invalid symbolic links.
- Modify link targets without resolving them.

Other changes:

- walk_test.ts: Rename `walkArray()` to `collectPaths()`
  because it is more descriptive.
- walk_test.ts: Add `collectEntries()`
@joehillen joehillen force-pushed the fix/1359/walk-return-links branch from 4c23486 to c76a54f Compare July 19, 2023 15:23
@joehillen
Copy link
Contributor Author

I think I fixed the test failure. I don't have access to windows, so I can't be sure until it runs again

@kt3k
Copy link
Member

kt3k commented Jul 27, 2023

Sorry for the delay in review.

After reading the original issue again, I think we should add includeSymlinks option instead of unconditionally returning the symlinks. We also need to check the matching of match, exts, skip before yielding.

Closing this one in favor of #3464, which implements includeSymlinks option.

Thanks for creating this PR anyway.

@kt3k kt3k closed this Jul 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

walk and walkSync skip over symlinks when followSymlinks: false
2 participants