search

szwacz / fs-jetpack

Better file system API for Node.js
MIT License
777 stars 41 forks source link

Make `jetpack.move` work cross-device #93

Closed papb closed 4 years ago

papb commented 4 years ago

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 commented 4 years ago

Will you be interested in updating this PR after changes made by https://github.com/szwacz/fs-jetpack/pull/92 ? :)

papb commented 4 years ago

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

papb commented 4 years ago

@szwacz PR updated! :smile:

szwacz commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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%) :arrow_down:

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 commented 4 years ago

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

papb commented 4 years ago

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