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

Edit walkSync to return items like what walk emits #310

Closed
manidlou opened this issue Nov 12, 2016 · 12 comments
Closed

Edit walkSync to return items like what walk emits #310

manidlou opened this issue Nov 12, 2016 · 12 comments

Comments

@manidlou
Copy link
Collaborator

What do you think about editing walkSync so that it would be consistent with walk? That is, walkSync also returns an array of items {path:, stats:} instead of only file paths, consistent with what walk emits as its data?

@RyanZim
Copy link
Collaborator

RyanZim commented Nov 12, 2016

@jprichardson ?

@manidlou
Copy link
Collaborator Author

Sorry, I just wanted to email @jprichardson to ask for his attention regarding this issue, but I accidentally email him through that PR. Bad mistake by me. Obviously that PR is not at all related to this issue. Please accept my apologies.

@jprichardson
Copy link
Owner

Yes, I like this a lot. However, since we just released 1.0.0, this wouldn't make it until 2.0.0 since it's a breaking change. I'd like to release 2.0.0 in early 2017 when we can drop Node v0.12 support.

@manidlou
Copy link
Collaborator Author

Absolutely. So, I will send you a PR. Then, whenever you have time, you can test it and see if it works the way that it is supposed to.

@r-murphy
Copy link

It would be great to also have directories included in the list as part of this change.

@RyanZim
Copy link
Collaborator

RyanZim commented Nov 16, 2016

This feature should be shipped in Jan 2017.

@jprichardson Thoughts on @r-murphy's proposal?

@jprichardson
Copy link
Owner

It would be great to also have directories included in the list as part of this change.

Gah, I think this is a bug and should be there. Maybe for 1.0 we consider walkSync() broken. IDK. Maybe for 2.0 walk() and walkSync() should be removed.

@RyanZim
Copy link
Collaborator

RyanZim commented Nov 16, 2016

I think this is a bug and should be there

Docs say:

Lists all files inside a directory recursively

I agree it's a bit unexpected though.

👍 for redoing or removing walk* for v2. I have thought it seems a bit out-of-scope for fs-extra (not saying we couldn't use it internally for copy). Not to mention the fact that it is sort of weird to have a non-callback async method.

@manidlou
Copy link
Collaborator Author

For walk(), if you are still willing to keep it, in order to make it consistent with other async methods, how about using klaw internally and when klaw emits end, we return a callback with an array of items {path:,stats:}?

@RyanZim
Copy link
Collaborator

RyanZim commented Dec 31, 2016

Fixed by #312 on the v2 branch. Still needs docs.

@manidlou
Copy link
Collaborator Author

@RyanZim I am preparing the docs to include the enhanced walkSync method.

@RyanZim
Copy link
Collaborator

RyanZim commented Jan 3, 2017

@jprichardson & I have decided to remove walk and walkSync from fs-extra, at least for the time being. See #338 for more details. Therefore, closing this issue.

@RyanZim RyanZim closed this as completed Jan 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants