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(which): match only executable files (#657) #874

Merged
merged 4 commits into from Jul 18, 2018

Conversation

termosa
Copy link
Contributor

@termosa termosa commented Jul 12, 2018

Fixes #657.

@termosa termosa force-pushed the b/657/which-finds-executables branch from 2369ccd to 644e632 Compare Jul 12, 2018
@codecov-io
Copy link

@codecov-io codecov-io commented Jul 12, 2018

Codecov Report

Merging #874 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #874      +/-   ##
=========================================
+ Coverage    95.8%   95.8%   +<.01%     
=========================================
  Files          34      34              
  Lines        1263    1264       +1     
=========================================
+ Hits         1210    1211       +1     
  Misses         53      53
Impacted Files Coverage Δ
src/which.js 95% <100%> (+0.12%) ⬆️

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 6d66a1a...ce70542. Read the comment docs.

Copy link
Member

@nfischer nfischer left a comment

Thanks for the great (and quick) PR!

test/which.js Outdated

test('None executable files does not appear in the result list', t => {
const commandName = 'node'; // Should be an existing command
const pathEnv = process.env.path || process.env.Path || process.env.PATH;
Copy link
Member

@nfischer nfischer Jul 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look right. This should be process.env.PATH.

Only all-caps $PATH works on unix. On Windows, process.env.path is totally the same as process.env.PATH (and all variations of capitalization of "path").

test/which.js Outdated
const matchingFile = path.resolve(extraPath, commandName);

// make sure that file is exists (will throw error otherwise)
fs.statSync(matchingFile);
Copy link
Member

@nfischer nfischer Jul 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can you make this t.truthy(fs.existsSync())? We generally prefer assertion failures over exceptions.

@@ -13,13 +13,25 @@ common.register('which', _which, {
// set on Windows.
var XP_DEFAULT_PATHEXT = '.com;.exe;.bat;.cmd;.vbs;.vbe;.js;.jse;.wsf;.wsh';

// For earlier versions of NodeJS that doesn't have a list of constants (< v6)
Copy link
Member

@nfischer nfischer Jul 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a TODO to use the node-provided constant on v6+? We'll eventually drop v4/5.

Copy link
Member

@nfischer nfischer Jul 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I'm trusting that (1) this value is respected on v4, even though the constant does not exist, and (2) the value is treated the same as F_OK on Windows, just as X_OK is documented:

Flag indicating that the file can be executed by the calling process. This has no effect on Windows (will behave like fs.constants.F_OK).

Copy link
Contributor Author

@termosa termosa Jul 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, on Windows X_OK and F_OK are equivalent. But on Windows platform file extensions are used to determine if file is executable or not.
I will not do this check for Windows platform.

Copy link
Member

@nfischer nfischer Jul 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will not do this check for Windows platform.

Sounds good

test/which.js Outdated
t.truthy(resultForWhichA);
t.truthy(resultForWhichA.length);
t.not(resultForWhich.toString(), matchingFile);
t.not(resultForWhichA[0], matchingFile);
Copy link
Member

@nfischer nfischer Jul 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you rewrite this as:

t.falsy(resultForWhichA.includes(matchingFile));

Also, I think it makes sense to skip this test on Windows, or to assert the array does contain the value on Windows. WDYT?

Copy link
Contributor Author

@termosa termosa Jul 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

includes is not handled by Ava and it is not supported in node@5. I would also save [0] index to make sure that order is proper.

@nfischer nfischer self-assigned this Jul 12, 2018
@termosa termosa force-pushed the b/657/which-finds-executables branch from 51fd66a to 9157b53 Compare Jul 14, 2018
Copy link
Member

@nfischer nfischer left a comment

LGTM % comment.

Also, can you check the coverage on your local machine? codecov says isExecutable() is not covered. If this is a codecov bug, can you add /* istanbul ignore next */ and add a comment mentioning codecov is to blame?

test/which.js Outdated
t.truthy(resultForWhichA.length);
t.not(resultForWhich.toString(), matchingFile);
// TODO(node-support): this can be used starting from node@6: t.falsy(resultForWhichA.includes(matchingFile))
t.not(resultForWhichA[0], matchingFile);
Copy link
Member

@nfischer nfischer Jul 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I thought .includes() was added by babel. Could you instead write this as:

t.is(resultForWhichA.indexOf(matchingFile), -1);

That way we have the correct semantics for the assertion.

@termosa termosa force-pushed the b/657/which-finds-executables branch from 25232fc to ce70542 Compare Jul 15, 2018
@termosa
Copy link
Contributor Author

@termosa termosa commented Jul 18, 2018

@nfischer done

t.not(resultForWhich.toString(), matchingFile);
t.is(resultForWhichA.indexOf(matchingFile), -1);

process.env.PATH = pathEnv;
Copy link
Member

@nfischer nfischer Jul 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry, I must have missed this. This should go in an test.afterEach.always block. Example:

shelljs/test/exec.js

Lines 14 to 17 in 6d66a1a

test.afterEach.always(() => {
process.chdir(CWD);
shell.config.execPath = ORIG_EXEC_PATH;
});

This shouldn't block this change. I added a comment to #834 to track this.

@nfischer nfischer merged commit 8dae55f into shelljs:master Jul 18, 2018
2 checks passed
@nfischer
Copy link
Member

@nfischer nfischer commented Jul 18, 2018

Thanks @termosa!

@termosa termosa deleted the b/657/which-finds-executables branch Jul 19, 2018
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.

None yet

3 participants