Skip to content

feature: add -a option for which command#655

Merged
nfischer merged 4 commits intoshelljs:masterfrom
termosa:master
Feb 10, 2017
Merged

feature: add -a option for which command#655
nfischer merged 4 commits intoshelljs:masterfrom
termosa:master

Conversation

@termosa
Copy link
Copy Markdown
Contributor

@termosa termosa commented Feb 4, 2017

With a -a option it will return an array of all matched binaries. Stdout is formatted as needed (joined with \n all binaries and \n is added to the end).
I've tested it on Mac OSX and Windows.

Copy link
Copy Markdown
Member

@nfischer nfischer left a comment

Choose a reason for hiding this comment

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

Looks pretty good! I have a couple questions:

  1. What gets returned if you get which('-a', 'alskdfjalskdfj')?
  2. Could we add a test that verifies the first result is always the same as which() (no options) would produce?

test/which.js Outdated
const result = shell.which('-a', 'node');
t.falsy(shell.error());
t.truthy(result);
t.truthy(result.length);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you check that result[0] === shell.which('node')?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@nfischer this is a good idea. I'll get it done.

@termosa
Copy link
Copy Markdown
Contributor Author

termosa commented Feb 9, 2017

@nfischer hi!

shell.which(command) returns String(path). Is it an expected result?
Currently, I've added a test that compares two strings but uses .toString for both before the compare, but it doesn't look well for me.

So let me know what do you think about String wrapper so I will either remove it or add it to the results of shell.which('-a', command).

Copy link
Copy Markdown
Member

@nfischer nfischer left a comment

Choose a reason for hiding this comment

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

@termosa thanks for the changes! They look fine, but you need to first fix the lint errors (if you run npm test it'll run the lint right after).

@nfischer
Copy link
Copy Markdown
Member

LGTM. I'll merge after CI passes

@nfischer nfischer merged commit 8dd2488 into shelljs:master Feb 10, 2017
@termosa
Copy link
Copy Markdown
Contributor Author

termosa commented Feb 11, 2017

Thank you, I'll be waiting for release.

@nfischer
Copy link
Copy Markdown
Member

I think we can make a v0.7.x release for this

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.

2 participants