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

Enhance walkSync() to return items with path and stats #312

Merged
merged 1 commit into from
Nov 16, 2016
Merged

Enhance walkSync() to return items with path and stats #312

merged 1 commit into from
Nov 16, 2016

Conversation

manidlou
Copy link
Collaborator

Hi,

This PR is regarding #310. I modified walkSync and its test to return an array of items {path:,stats:} to be consistent with walk. Please let me know what you think. I appreciate it in advance. Thanks a lot.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 85.473% when pulling fca1d7b on mawni:enhance-walkSync into b96e003 on jprichardson:master.

} else {
filelist.push(nestedPath)
list.push({path: nestedPath, stats: assign({}, {type: 'file'}, stat)})
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the type field?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I forgot to tell you that I added that just to make it more distinct. If you think that's unnecessary, I remove it right away.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I remove it then?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I remove it then?

Yea, as it doesn't match fs.walk().

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For sure. I'll send you another PR, then. Thanks @jprichardson. I really appreciate your support.

Copy link
Collaborator Author

@manidlou manidlou Nov 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, the current version of walkSync doesn't catch the fs.readdirSync() and fs.lstatSync() errors if any been thrown. What do you think about wrapping the function in a try/catch block and throw the error if any occurred?

Edit:
Since it is clearly mentioned in the docs that the Sync version of the functions throws the errors, I think we can ignore catching errors here since fs will throw them anyway. Right?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 85.457% when pulling 9df7ca0 on mawni:enhance-walkSync into b96e003 on jprichardson:master.

@RyanZim
Copy link
Collaborator

RyanZim commented Nov 14, 2016

@jprichardson Can you manually restart the appveyor build?

@jprichardson
Copy link
Owner

@jprichardson Can you manually restart the appveyor build?

I wonder why it's failing here... I think a push has to trigger it...

@RyanZim
Copy link
Collaborator

RyanZim commented Nov 14, 2016

@jprichardson I think it migh be related to isaacs/node-graceful-fs#98.

@RyanZim
Copy link
Collaborator

RyanZim commented Nov 15, 2016

@mawni Can you rebase onto master. The issue with windows tests should be fixed.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 85.44% when pulling 501e02d on mawni:enhance-walkSync into b96e003 on jprichardson:master.

@RyanZim
Copy link
Collaborator

RyanZim commented Nov 15, 2016

@jprichardson Are the changes introduced here merge-ready? If so, I will cherry-pick this to master.

@jprichardson
Copy link
Owner

@mawni will you squash these?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 85.44% when pulling 3642289 on mawni:enhance-walkSync into ac89f5a on jprichardson:master.

@RyanZim RyanZim added this to the 2.0.0 milestone Nov 16, 2016
@jprichardson
Copy link
Owner

@RyanZim for now, I've created a branch https://github.com/jprichardson/node-fs-extra/tree/1.x in case we need to publish any hotfixes. master can be our new development branch. We'll probably need to rename these branches at some point to make it clear about what's going on.

@jprichardson jprichardson merged commit 3345e63 into jprichardson:master Nov 16, 2016
@jprichardson
Copy link
Owner

@mawni thanks! Will be published in early Jan.

@RyanZim
Copy link
Collaborator

RyanZim commented Nov 16, 2016

@jprichardson This needs to be documented sometime before the release; however, we don't really want incorrect docs on master. Advice?

@jprichardson
Copy link
Owner

This needs to be documented sometime before the release; however, we don't really want incorrect docs on master. Advice?

I think we should have a docs/ folder with the README being lite... maybe with just a ToC. This would point to the actual docs. Or, I can also change the default branch. This PR didn't change the docs anyways, so we're okay for now. I'm open to any other ideas though.

@RyanZim
Copy link
Collaborator

RyanZim commented Nov 16, 2016

@jprichardson If you want docs/, go for it!

As far as branching, I prefer master to be the latest non-breaking changes, and a long-running develop branch for the next major release (merging into master just prior to the release).

Since this is already merged, how about changing 1.x to be the default branch and using master for v2? When we are ready to release v2, we can change the default back. Thoughts?

@jprichardson
Copy link
Owner

As far as branching, I prefer master to be the latest non-breaking changes, and a long-running develop branch for the next major release (merging into master just prior to the release).

I like this. It's what I do for private bigger projects.

Since this is already merged, how about changing 1.x to be the default branch and using master for v2? When we are ready to release v2, we can change the default back. Thoughts?

I'm okay with this, do you mind spearheading it?

@RyanZim
Copy link
Collaborator

RyanZim commented Nov 16, 2016

@jprichardson Only the owner can change the default branch, so you'll have to do that. As far as v2, I'll try to shoot you an email when I get a chance.

@jprichardson
Copy link
Owner

@jprichardson Only the owner can change the default branch, so you'll have to do that. As far as v2, I'll try to shoot you an email when I get a chance.

done!

@manidlou manidlou deleted the enhance-walkSync branch December 31, 2016 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants