szwacz / fs-jetpack

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

added ignoreCase option to find, findAsync, copy, and copyAsync #74

Closed matortheeternal closed 6 years ago

matortheeternal commented 6 years ago

Addresses #73.

codecov-io commented 6 years ago

Codecov Report

Merging #74 into master will increase coverage by 0.01%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #74      +/-   ##
==========================================
+ Coverage   96.34%   96.35%   +0.01%     
==========================================
  Files          24       24              
  Lines        1258     1263       +5     
  Branches      239      242       +3     
==========================================
+ Hits         1212     1217       +5     
  Misses         46       46
Impacted Files Coverage Δ
lib/copy.js 95.12% <100%> (+0.06%) :arrow_up:
lib/find.js 100% <100%> (ø) :arrow_up:
lib/utils/matcher.js 100% <100%> (ø) :arrow_up:

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 75352ad...c4ceeb1. Read the comment docs.

szwacz commented 6 years ago

Sorry, I'm quite busy right now. Will do my best to review this at the weekend.

matortheeternal commented 6 years ago

@szwacz all good, take your time. :+1:

szwacz commented 6 years ago

Excellet work @matortheeternal. I've merged it into master, run tests on my macOS... and realized the obvious, that macOS file system by default is case insensitive as well :) But for everything to be more tricky, it might be, but you can change it: https://en.wikipedia.org/wiki/Case_sensitivity Also NTFS is case-sensitive according to that wikipedia entry.

Long story short, the problem is that case sensitivity is a feature of file system, not operating system. So by setting it to default value depending on OS will inevitably lead to edge cases. Must do more research (any help with that will be appreciated :) ).

I'm only concerned if the default value should be picked depending on OS. The rest of your code is great!

szwacz commented 6 years ago

There you have it: https://nodejs.org/en/docs/guides/working-with-different-filesystems/

Be wary of inferring filesystem behavior from process.platform. For example, do not assume that because your program is running on Darwin that you are therefore working on a case-insensitive filesystem (HFS+), as the user may be using a case-sensitive filesystem (HFSX). Similarly, do not assume that because your program is running on Linux that you are therefore working on a filesystem which supports Unix permissions and inodes, as you may be on a particular external drive, USB or network drive which does not.

szwacz commented 6 years ago

Acutally that article makes it even more tricky:

You may create a directory called test/abc and be surprised to see sometimes that fs.readdir('test') returns ['ABC']. This is not a bug in Node. Node returns the filename as the filesystem stores it, and not all filesystems support case-preservation. Some filesystems convert all filenames to uppercase (or lowercase).

szwacz commented 6 years ago

Published this patch as v2.2.0, but had to simplify it a bit. Platform detection for setting default case-sensitivity just won't work everywhere.

Thank you for your help @matortheeternal !

matortheeternal commented 6 years ago

Alright, sounds good :+1: