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 for Node Path instance not being sent to onEachFile and onEachDirectory callbacks #87

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

photonstorm
Copy link

The README says: "The callback function takes the directory item (has path, name, size, and extension) and an instance of node path and an instance of node FS.stats."

However, the callbacks are not sent the Node Path instance but instead are sent the path variable.

Changing from path to PATH fixes it to work as described in the README.

…ectory callbacks

The README says: "The callback function takes the directory item (has path, name, size, and extension) and an instance of [node path](https://nodejs.org/api/path.html) and an instance of [node FS.stats](https://nodejs.org/api/fs.html#fs_class_fs_stats)."

However, the callbacks are not sent the Node Path instance but instead are sent the `path` variable.

Changing from `path` to `PATH` fixes it to work as described in the README.
@mihneadb
Copy link
Owner

mihneadb commented May 4, 2021

@photonstorm thanks for the PR! Going over this I was actually thinking - why do we even need the path module passed as an argument to the callback function?

I went through git history and it was added like that from a contribution, but really - do we need it? Seems to complicate things. I'm guessing if one needs the node path module they can simply require it in their code, no?

(If you agree, perhaps we could turn around this PR into actually removing that arg altogether.)

Thanks!

@photonstorm
Copy link
Author

I agree, I don't think it's needed at all. If you need path or fs stats you can just import it yourself.

Happy to close this PR and do a new one if you just want to remove it entirely.

@mihneadb
Copy link
Owner

mihneadb commented May 4, 2021 via email

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