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

Add stdin and input handling #22

Closed
wants to merge 2 commits into from
Closed

Add stdin and input handling #22

wants to merge 2 commits into from

Conversation

SamVerschueren
Copy link
Contributor

fixes #20

Feedback is more then welcome.

Note: PR is made from branch iss17.

@SamVerschueren
Copy link
Contributor Author

Will fix the tests tomorrow. Not much time at the moment.

@sindresorhus
Copy link
Owner

Can you rebase when you have a chance?

@SamVerschueren
Copy link
Contributor Author

I might have done something wrong. I'm not able to squash those commits. The first commit also has the changes of the previous PR (#19 - add type inference option) which I merged from master. You're probably way better in it then me :).

@SamVerschueren
Copy link
Contributor Author

@sindresorhus ping

I know you're quite busy lately, just reply when you find the time.

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

@sindresorhus
Copy link
Owner

There are two more use-cases. Just getting stdin without input, and getting stdin or file. Maybe we should also add: .stdin() (just exposing get-stdin) and .stdinOrFile() (which would fs.readFileSync the input). Any ideas on how could simplify this. Not sure how I feel about adding three methods. Are we taking the right approach here?


cli.stdinOrInput()
.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.

@SamVerschueren
Copy link
Contributor Author

Any ideas on how could simplify this. Not sure how I feel about adding three methods. Are we taking the right approach here?

Good point, it makes the module more complex. Not sure what you think about a wrapper like meow-stdin which could wrap meow and add all these extra methods.

const meowStdin = require('meow-stdin');
const meow = meowStdin(require('meow'));

// and so on

Or a separate package that uses meow in the background but adds stdin methods.

const meow = require('meow-stdin');

// and so on

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.

sindresorhus referenced this pull request in vadimdemedes/cssor Jan 24, 2016

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

global.Promise = Promise;

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.

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.

stdin handling
5 participants