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

Don't move when source and dest paths are the same. #378

Merged
merged 1 commit into from
Mar 14, 2017
Merged

Don't move when source and dest paths are the same. #378

merged 1 commit into from
Mar 14, 2017

Conversation

jdalton
Copy link
Contributor

@jdalton jdalton commented Mar 3, 2017

This PR addresses #377 to avoid a move operation if the source and dest paths are the same.

(I'll add tests too, just wanted to get the ball rolling)

if (overwrite) {
if (path.resolve(source) === path.resolve(dest)) {
setTimeout(callback, 1)
} else if (overwrite) {
Copy link
Contributor Author

@jdalton jdalton Mar 3, 2017

Choose a reason for hiding this comment

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

fs.move is async do you want it to exit early as such too (the setTimeout).
Do y'all have a pattern in place for this already?

Copy link
Owner

Choose a reason for hiding this comment

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

@jdalton what are your thoughts on setTimeout over setImmediate? It's my understanding setTimeout with an input of 1, still clamps to 15 ms.

Copy link
Contributor Author

@jdalton jdalton Mar 6, 2017

Choose a reason for hiding this comment

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

what are your thoughts on setTimeout over setImmediate? It's my understanding setTimeout with an input of 1, still clamps to 15 ms.

I have no strong opinions on it here since the primary goal is to just defer it a bit.
However, based on that goal setImmediate does look like a better call.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 85.352% when pulling 50ecc8f on jdalton:move-check into e771770 on jprichardson:master.

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, will approve when tests are added.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.09%) to 79.435% when pulling b74cf5b on jdalton:move-check into e06440b on jprichardson:master.

@jprichardson
Copy link
Owner

Before the behavior would error out, right? Should we consider this breaking? Thoughts @jdalton?

@jdalton
Copy link
Contributor Author

jdalton commented Mar 8, 2017

It did error out. I would consider this a bug fix. (I haven't forgotten about the unit tests, just busy)

@manidlou
Copy link
Collaborator

manidlou commented Mar 8, 2017

What should be the correct behavior then? Since whatever that'd be, the same behavior should be applied to moveSync as well.

@jdalton
Copy link
Contributor Author

jdalton commented Mar 8, 2017

What should be the correct behavior then? Since whatever that'd be, the same behavior should be applied to moveSync as well.

I'm asserting by the previous discussion, title, and behavior of this PR "Don't move when source and dest paths are the same."

@jdalton
Copy link
Contributor Author

jdalton commented Mar 13, 2017

Added tests.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 80.756% when pulling 8d978db on jdalton:move-check into cf2c50d on jprichardson:master.

@jprichardson jprichardson merged commit efe0d80 into jprichardson:master Mar 14, 2017
@jprichardson
Copy link
Owner

Thank you @jdalton!

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.

5 participants