Skip to content

Read from STDIN if data is being piped in#135

Merged
puzrin merged 1 commit intonodeca:masterfrom
sethfitz:stdin
Jul 26, 2014
Merged

Read from STDIN if data is being piped in#135
puzrin merged 1 commit intonodeca:masterfrom
sethfitz:stdin

Conversation

@sethfitz
Copy link
Copy Markdown
Contributor

if (process.stdin.isTTY) {
  cli.addArgument(['file'], {
    help:   'File to read, utf-8 encoded without BOM'
  });
}

...was a cheeky way of making file optional (I couldn't figure out how to make positional arguments optional with argparse). However, a side-effect of this is that - isn't a valid way to instruct js-yaml to read from STDIN. I'm happy to amend the patch if you have suggestions on how to achieve this.

(This addresses #130)

@puzrin
Copy link
Copy Markdown
Member

puzrin commented Jul 24, 2014

Did you googled fo "argparse stdin" ? Seems stackoverflow has some answers.

@sethfitz
Copy link
Copy Markdown
Contributor Author

The fundamental problem was that file needs to be an optional argument; nargs: '?' works.

@puzrin
Copy link
Copy Markdown
Member

puzrin commented Jul 24, 2014

So, why you did not used nargs with default to '-' or something like that ?

@sethfitz
Copy link
Copy Markdown
Contributor Author

Check 808373e, which I just attached to this PR.

@puzrin
Copy link
Copy Markdown
Member

puzrin commented Jul 24, 2014

Please, rebase - squash into one commit

@sethfitz
Copy link
Copy Markdown
Contributor Author

Done.

@puzrin
Copy link
Copy Markdown
Member

puzrin commented Jul 24, 2014

What happens if no file defined, and no data piped via < or "|" ?

Preliminary looks ok for me

@sethfitz
Copy link
Copy Markdown
Contributor Author

It reads from STDIN until it sees an EOF (^d). That's consistent with cat's behavior.

Comment thread bin/js-yaml.js Outdated
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This should be function readFile(...) {

@dervus
Copy link
Copy Markdown
Collaborator

dervus commented Jul 25, 2014

Except the remark, it looks good for me.

@sethfitz
Copy link
Copy Markdown
Contributor Author

Changed, thanks.

puzrin pushed a commit that referenced this pull request Jul 26, 2014
Read from STDIN if data is being piped in
@puzrin puzrin merged commit 2aed435 into nodeca:master Jul 26, 2014
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.

3 participants