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

Make jetpack.move work cross-device #93

Merged
merged 1 commit into from
Jul 19, 2020
Merged

Make jetpack.move work cross-device #93

merged 1 commit into from
Jul 19, 2020

Conversation

papb
Copy link
Contributor

@papb papb commented Jul 12, 2020

Closes #48
Closes #88

The behavior I chose for when the destination already exists is the same currently happening when in the same device. See #92.

I did not add a test because I don't think it's possible to test for EXDEV in travis/appveyor. I tested manually though.

@szwacz
Copy link
Owner

szwacz commented Jul 15, 2020

Will you be interested in updating this PR after changes made by #92 ? :)

@papb
Copy link
Contributor Author

papb commented Jul 15, 2020

@szwacz Yes, will do as soon as possible, thank you!!

lib/move.js Outdated Show resolved Hide resolved
@papb
Copy link
Contributor Author

papb commented Jul 16, 2020

@szwacz PR updated! 😄

@szwacz
Copy link
Owner

szwacz commented Jul 17, 2020

The code looks good :)

There are some problems with node v14 on windows: https://ci.appveyor.com/project/szwacz/fs-jetpack/build/job/2gl5f46j3pm7akcl

I'd also need to test it on MacOS/Linux (I believe you're a Windows user so it was already tested there).

@papb
Copy link
Contributor Author

papb commented Jul 17, 2020

@szwacz Would you like me to add a test by mocking fs.rename and fs.renameSync and making them throw 'EXDEV'?

Also looks like Travis did not run, any idea why?

Also appveyor is not helpful with the Node 14 failure, unless I am missing something, it just says Command exited with code -1073741819 with no log whatsoever... Do you have any idea why? Perhaps just re-running appveyor might fix it?

@szwacz
Copy link
Owner

szwacz commented Jul 17, 2020

I don't think this is good library to use mocks. After all the whole purpose of it is to change the state of file system, if you mock the file system then the most essential element was thrown away, and the remaining test tests very little. Tough luck, this part just won't be automatically tested.

Travis did run, just for some reason doesn't appear here: https://travis-ci.org/github/szwacz/fs-jetpack

Yes, the error with node 14 is wierd. Do you have this version installed on your windows machine and the tests pass?

@szwacz
Copy link
Owner

szwacz commented Jul 17, 2020

Ok, the AppVeyor error looks like their testing environment issue, and not issue of this library. Actually Travis is supporting now Windows builds: https://docs.travis-ci.com/user/multi-os/ I'll gieve it a try.

@codecov-commenter
Copy link

Codecov Report

Merging #93 into master will decrease coverage by 0.31%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #93      +/-   ##
==========================================
- Coverage   97.37%   97.05%   -0.32%     
==========================================
  Files          24       24              
  Lines        1256     1258       +2     
  Branches      240      237       -3     
==========================================
- Hits         1223     1221       -2     
- Misses         33       37       +4     
Impacted Files Coverage Δ
lib/move.js 90.14% <66.66%> (-5.52%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8aaa189...a37c218. Read the comment docs.

@szwacz szwacz merged commit 27ac63a into szwacz:master Jul 19, 2020
@szwacz
Copy link
Owner

szwacz commented Jul 19, 2020

I've tested it on MacOS, looks good, so released as v3.1.0

@papb
Copy link
Contributor Author

papb commented Jul 19, 2020

Awesome, sorry for not replying earlier. Yes, works for me on windows 😁

@papb papb deleted the fix-move-exdev branch July 19, 2020 19:43
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.

move does not work across partitions/devices Can't cross device move file on windows platform.
4 participants