search

baudehlo / node-fs-ext

Extras missing from node's fs module
MIT License
101 stars 45 forks source link

Fixes for Windows #70

Closed davedoesdev closed 6 years ago

davedoesdev commented 6 years ago

Tests now pass and it compiles on Windows!

Question:

codecov-io commented 6 years ago

Codecov Report

Merging #70 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #70   +/-   ##
=======================================
  Coverage   84.33%   84.33%           
=======================================
  Files           1        1           
  Lines          83       83           
  Branches       24       24           
=======================================
  Hits           70       70           
  Misses         13       13

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 808593c...f2b5e9f. Read the comment docs.

davedoesdev commented 6 years ago

Appveyor doesn't seem to be picking up change to run on Node 8 EDIT: fixed

davedoesdev commented 6 years ago

Travis and Appveyor both building successfully.

davedoesdev commented 6 years ago

Works on Node 8.1.3. Might work on earlier versions, not sure when https://github.com/libuv/libuv/pull/1323 went into Node.

davedoesdev commented 6 years ago

@baudehlo sure see latest commit

davedoesdev commented 6 years ago

@baudehlo is there anything else you need on this PR?

baudehlo commented 6 years ago

I don't think so. Just need some free time (baby, new house, etc).

On Jul 19, 2017, at 6:46 PM, David Halls notifications@github.com wrote:

@baudehlo is there anything else you need on this PR?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

davedoesdev commented 6 years ago

@baudehlo been there - take time for what's important.

baudehlo commented 6 years ago

Merged

baudehlo commented 6 years ago

BTW after merge Travis fails. Not sure why.

On Thu, Jul 20, 2017 at 1:40 AM, David Halls notifications@github.com wrote:

@baudehlo https://github.com/baudehlo been there - take time for what's important.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/baudehlo/node-fs-ext/pull/70#issuecomment-316602289, or mute the thread https://github.com/notifications/unsubscribe-auth/AAobY7u3RPnzhcNlT4d1MrvrEyxTMz21ks5sPuhHgaJpZM4ORcVU .

davedoesdev commented 6 years ago

Node 8 behaviour has changed for the -1 utime test. It sets it fine (and Node 6 and Python read the atime as -1). But for Node 8 atime in that case is NaN.

davedoesdev commented 6 years ago

@baudehlo how do utime and utimeSync provided by fs-ext differ from fs.futimes and fs.futimesSync in Node core?

baudehlo commented 6 years ago

I think those were just added after I released this.

On Jul 20, 2017, at 6:18 PM, David Halls notifications@github.com wrote:

@baudehlo how do utime and utimeSync provided by fs-ext differ from fs.futimes and fs.futimesSync don't?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

davedoesdev commented 6 years ago

How would you feel about removing them or making them aliases?

On 20 Jul 2017 11:52 p.m., "Matt Sergeant" notifications@github.com wrote:

I think those were just added after I released this.

On Jul 20, 2017, at 6:18 PM, David Halls notifications@github.com wrote:

@baudehlo how do utime and utimeSync provided by fs-ext differ from fs.futimes and fs.futimesSync don't?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/baudehlo/node-fs-ext/pull/70#issuecomment-316852590, or mute the thread https://github.com/notifications/unsubscribe-auth/ACOMU8bjBcJ_GS_8R1A8NCVss3-rd0srks5sP9oZgaJpZM4ORcVU .

baudehlo commented 6 years ago

Sounds good to me.

On Jul 20, 2017, at 7:00 PM, David Halls notifications@github.com wrote:

How would you feel about removing them or making them aliases?

On 20 Jul 2017 11:52 p.m., "Matt Sergeant" notifications@github.com wrote:

I think those were just added after I released this.

On Jul 20, 2017, at 6:18 PM, David Halls notifications@github.com wrote:

@baudehlo how do utime and utimeSync provided by fs-ext differ from fs.futimes and fs.futimesSync don't?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/baudehlo/node-fs-ext/pull/70#issuecomment-316852590, or mute the thread https://github.com/notifications/unsubscribe-auth/ACOMU8bjBcJ_GS_8R1A8NCVss3-rd0srks5sP9oZgaJpZM4ORcVU .

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

davedoesdev commented 6 years ago

71