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

[BREAKING] moveSync: refactor to use renameSync #609

Merged
merged 1 commit into from Aug 12, 2018
Merged

Conversation

manidlou
Copy link
Collaborator

@manidlou manidlou commented Aug 4, 2018

fix #608.

Refactored moveSync to be consistent with move. Also removed unnecessary rimraf package from dev-deps.

@coveralls
Copy link

coveralls commented Aug 4, 2018

Coverage Status

Coverage decreased (-0.2%) to 86.019% when pulling 95a9c72 on fix-move-sync into fe516c3 on v8-dev.

@manidlou
Copy link
Collaborator Author

manidlou commented Aug 4, 2018

@RyanZim I was thinking maybe that'd better we follow your good pattern of creating v8-dev branch and include this one (I will update the PR base branch) as well as #587 to v8 release. Also, I will have more PRs to do some cleanups 😁 tho. So we can include them all to v8 release?

@manidlou manidlou changed the base branch from master to v8-dev August 5, 2018 06:37
@manidlou
Copy link
Collaborator Author

manidlou commented Aug 5, 2018

I changed the base to v8-dev.

Copy link
Collaborator

@JPeer264 JPeer264 left a comment

Choose a reason for hiding this comment

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

Could not think of any addition. Also nice cleanups in the tests 👏

Copy link
Collaborator

@RyanZim RyanZim left a comment

Choose a reason for hiding this comment

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

Code LGTM

@RyanZim
Copy link
Collaborator

RyanZim commented Aug 9, 2018

This will need rebased to fix merge conflicts.

@manidlou you've been very helpful here at fs-extra, and too often, I feel like I'm the bottleneck with the release process. As a result, I've added you to fs-extra on npm (i.e. you now have npm publish rights). Of course, as always, we still should get peer review before merging our own PRs. If you have any questions, or need any help with getting a release out, I'm always here, can ask me questions on GH or write me at opensrc@ryanzim.com, whichever you prefer.

@manidlou
Copy link
Collaborator Author

@RyanZim thanks a lot for giving me publish rights. I am truly honored to be part of fs-extra team! I also appreciate your openness for offering help. Just to confirm with you, when I run npm owner ls fs-extra, it only returns you and @jprichardson. IDK if it should include me as well or there is another way of checking this?!

You are right about the conflict. I'll rebase this.

@manidlou
Copy link
Collaborator Author

Rebased!

@RyanZim
Copy link
Collaborator

RyanZim commented Aug 10, 2018

@manidlou Sorry, seems npm was a little buggy and didn't give you access when I requested it. Now fixed; you should have access.

@manidlou
Copy link
Collaborator Author

manidlou commented Aug 10, 2018

Thanks @RyanZim I appreciate it!

@manidlou
Copy link
Collaborator Author

@jprichardson are you ok with this?! So that I can merge this since move*() still needs some work. I am gonna apply another fix to move*() to handle same paths correctly. But this needs to be merged before that one comes in! 😁

@RyanZim RyanZim added this to the 8.0.0 milestone Aug 11, 2018
@jprichardson
Copy link
Owner

@manidlou yep, I'm okay with it

@manidlou manidlou merged commit e5cbdc4 into v8-dev Aug 12, 2018
@manidlou manidlou deleted the fix-move-sync branch August 12, 2018 00:17
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.

None yet

5 participants