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

Fix issue-585: Remove duplicate stat.h constants defined in constants.js #609

Merged
merged 3 commits into from Dec 13, 2018

Conversation

@yuzhouChen
Copy link
Contributor

yuzhouChen commented Dec 4, 2018

Fix issue-585
After removing

  S_IFREG: 0x8000,
  S_IFDIR: 0x4000,
  S_IFLNK: 0xA000,

all tests are passed. Because I fixed the both issue-588 and issue-585, I found out that when I have the code without the fix issue-588, when I run npm test command, it cannot launch Firefox, as show in image below:

screen shot 2018-12-04 at 6 05 05 pm

So I think maybe have a relation between them. Just let you know that.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 4, 2018

Codecov Report

Merging #609 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #609   +/-   ##
=======================================
  Coverage   86.16%   86.16%           
=======================================
  Files          16       16           
  Lines        1720     1720           
=======================================
  Hits         1482     1482           
  Misses        238      238
Impacted Files Coverage Δ
src/constants.js 100% <ø> (ø) ⬆️
src/node.js 91.3% <100%> (+0.19%) ⬆️
src/filesystem/implementation.js 83.19% <0%> (-0.02%) ⬇️

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 0f93a04...5519e7c. Read the comment docs.

Copy link
Contributor

humphd left a comment

You also need to fix the callers in src/node.js to use the correct version on Constants.fsConstants now that these are gone. You can git grep for them to see what I mean:

$ git grep -nC 3 "S_IFREG" src
src/constants.js-24-  NODE_TYPE_SYMBOLIC_LINK: 'SYMLINK',
src/constants.js-25-  NODE_TYPE_META: 'META',
src/constants.js-26-
src/constants.js:27:  S_IFREG: 0x8000,
src/constants.js-28-  S_IFDIR: 0x4000,
src/constants.js-29-  S_IFLNK: 0xA000,
src/constants.js-30-
--
src/constants.js-93-    O_WRONLY: 1,
src/constants.js-94-    O_RDWR: 2,
src/constants.js-95-    S_IFMT: 61440,
src/constants.js:96:    S_IFREG: 32768,
src/constants.js-97-    S_IFDIR: 16384,
src/constants.js-98-    S_IFCHR: 8192,
src/constants.js-99-    S_IFBLK: 24576,
--
src/node.js-2-var NODE_TYPE_DIRECTORY = require('./constants.js').NODE_TYPE_DIRECTORY;
src/node.js-3-var NODE_TYPE_SYMBOLIC_LINK = require('./constants.js').NODE_TYPE_SYMBOLIC_LINK;
src/node.js-4-
src/node.js:5:var S_IFREG = require('./constants.js').S_IFREG;
src/node.js-6-var S_IFDIR = require('./constants.js').S_IFDIR;
src/node.js-7-var S_IFLNK = require('./constants.js').S_IFLNK;
src/node.js-8-
--
src/node.js-19-  case NODE_TYPE_FILE:
src/node.js-20-    // falls through
src/node.js-21-  default:
src/node.js:22:    return (mode || DEFAULT_FILE_PERMISSIONS) | S_IFREG;
src/node.js-23-  }
src/node.js-24-}
src/node.js-25-
@humphd

This comment has been minimized.

Copy link
Contributor

humphd commented Dec 5, 2018

You need to run npm install and also have Firefox installed to run Firefox headless.

@yuzhouChen

This comment has been minimized.

Copy link
Contributor Author

yuzhouChen commented Dec 9, 2018

Fixed the callers in src/node.js to the correct version, let me know if I have done something wrong.

chen yuzhou chen yuzhou
@humphd
humphd approved these changes Dec 13, 2018
@humphd humphd merged commit 3d7ff3e into filerjs:master Dec 13, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.