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.

Sign up for GitHub

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

Jump to bottom

Add stdin and input handling #22

Closed
wants to merge 2 commits into from
Closed

Add stdin and input handling #22

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
af1f9dc
add stdin or input handling
SamVerschueren Dec 16, 2015
a84b4e4
fix test
SamVerschueren Dec 16, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions fixture.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,14 @@ var cli = meow({
Object.keys(cli.flags).forEach(function (el) {
console.log(el);
});

cli.stdinOrInput()
Copy link

Choose a reason for hiding this comment

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

Maybe cli.autoInput() or cli.getInput() or something along those lines would look better and shorter?

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 for .getInput(), even though it might be kinda general and it's intent unclear. If it's well documented I think that solves that, and it's easier to remember.

Copy link
Owner

@sindresorhus sindresorhus Jul 10, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

👍 .getInput().

.then(function (input) {
input.forEach(function (element) {
Copy link
Owner

Choose a reason for hiding this comment

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

The main use-case is one input argument, so would really like to optimize for that, but still support the edge-case of multiple arguments.

console.log(element);
});
})
.catch(function (err) {
console.error(err.message);
process.exit(1);
});
19 changes: 18 additions & 1 deletion index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,15 @@ var redent = require('redent');
var readPkgUp = require('read-pkg-up');
var loudRejection = require('loud-rejection');
var normalizePackageData = require('normalize-package-data');
var getStdin = require('get-stdin');
var Promise = require('pinkie-promise');

// get the uncached parent
delete require.cache[__filename];
var parentDir = path.dirname(module.parent.filename);

global.Promise = Promise;
Copy link

Choose a reason for hiding this comment

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

Isn't it against the rules or am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

get-stdin does not support 0.10 and thus does not come bundled with pinkie-promise. So either we drop 0.10 in meow, or add pinkie-promise to get-stdin in order to get rid of this.


module.exports = function (opts, minimistOpts) {
loudRejection();

Expand Down Expand Up @@ -80,6 +84,19 @@ module.exports = function (opts, minimistOpts) {
flags: camelcaseKeys(argv),
pkg: pkg,
help: help,
showHelp: showHelp
showHelp: showHelp,
stdinOrInput: function () {
if (!_ && process.stdin.isTTY) {
return Promise.reject(new Error('input required'));
Copy link
Owner

Choose a reason for hiding this comment

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

input => Input

}

if (_.length > 0) {
return Promise.resolve(_);
}

return getStdin().then(function (data) {
return [data];
Copy link

Choose a reason for hiding this comment

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

Why is array returned instead of data?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because _ is an array as well. This way you can just iterate over the result instead of checking Array.isArray.

});
}
};
};
2 changes: 2 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,12 @@
],
"dependencies": {
"camelcase-keys": "^2.0.0",
"get-stdin": "^5.0.1",
"loud-rejection": "^1.0.0",
"minimist": "^1.1.3",
"normalize-package-data": "^2.3.4",
"object-assign": "^4.0.1",
"pinkie-promise": "^2.0.0",
"read-pkg-up": "^1.0.1",
"redent": "^1.0.0",
"trim-newlines": "^1.0.0"
Expand Down
12 changes: 10 additions & 2 deletions test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,9 @@ test('spawn cli and show help screen', async t => {
});

test('spawn cli and test input', async t => {
const {stdout} = await execa('./fixture.js', ['-u', 'cat']);
const {stdout} = await execa('./fixture.js', ['foo', '-u', 'cat']);

t.is(stdout, 'u\nunicorn\nmeow');
t.is(stdout, 'u\nunicorn\nmeow\nfoo');
});

test.serial('pkg.bin as a string should work', t => {
Expand All @@ -71,3 +71,11 @@ test('type inference', t => {
t.is(fn({argv: ['5'], inferType: true}, {string: ['foo']}).input[0], 5);
t.is(fn({argv: ['5'], inferType: true}, {string: ['_', 'foo']}).input[0], 5);
});

test('stdinOrInput', async t => {
const stdin = await execa.shell('echo \'foo bar\' | ./fixture.js');
const input = await execa('./fixture.js', ['foo', 'bar']);

t.is(stdin.stdout, 'meow\nfoo bar\n');
t.is(input.stdout, 'meow\nfoo\nbar');
Copy link

Choose a reason for hiding this comment

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

This isn't how stdin is intended. At least, this is completely counterintuitive for any other program out there. Usually how it works is program file.txt or cat file.txt | program.

This assumes the command line input is instead input as well. This will confuse a ton of people if Meow's code handling assumes this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a big difference in echo "foo bar" | cli and cat file.txt | cli where the content of file.txt is foo bar? I assumed, if I test echo "foo bar" | cli, then cat file.txt | cli will work as well?

Copy link

Choose a reason for hiding this comment

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

That's not what I mean. Usually programs that accept filenames as arguments are the ones that employ the use of stdin.

For instance:

cat > some-file.txt <<EOF
some text
EOF

grep "text" some-file.txt
echo "some text" | grep "text"

sed 's/some/thing/' some-file.txt
echo "some text" | sed 's/some/thing/'

Instead, this PR seems to support the usage of:

cat > some-prog.js <<EOF
// ...
console.log(meow.input);
// ...
EOF

some-prog.js This Is Some Text
# doing the same as
echo This Is Some Text | some-prog.js

Which is going to confuse a lot of people.

Copy link
Owner

Choose a reason for hiding this comment

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

Usually programs that accept filenames as arguments are the ones that employ the use of stdin.

Sure, but I also often use it for CLI's that accept text input too, to make it easier for everyone to use it.

Then they could use either of these:

$ cli-app foo
$ echo foo | cli-app

Copy link
Owner

Choose a reason for hiding this comment

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

Actually, I take back my last comment. Better to use stdin for just string input. No point in doing both, and stdin should be preferred as it works nicely in a pipe chain.

});