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

dont open stdin by default #15

Merged
merged 1 commit into from Apr 6, 2015

Conversation

4 participants
@maxogden
Contributor

maxogden commented Oct 21, 2014

this makes a breaking change I think

me and @mafintosh were trying to figure out why this was hanging:

bionode ncbi search genome eukaryota | dat import --json

and it was because of how bionode was always doing process.stdin.pipe(cli.stdin).

the more unixy way to do it would be to require a - as the last argument, which is what this implements, so now if you want bionode to read from stdin you can do:

bionode ncbi search genome eukaryota

otherwise it won't open stdin

@maxogden

This comment has been minimized.

Contributor

maxogden commented Oct 21, 2014

ah, this causes errors like this:

events.js:72
        throw er; // Unhandled 'error' event
              ^
Error: 2014-10-21T08:22:18 fastq-dump.2.3.5 err: param incorrect while reading argument list within application support module - -

    at Socket.<anonymous> (/Users/maxogden/src/js/bionode/node_modules/bionode-sra/lib/bionode-sra.js:105:28)
    at Socket.EventEmitter.emit (events.js:95:17)
    at Socket.<anonymous> (_stream_readable.js:745:14)
    at Socket.EventEmitter.emit (events.js:92:17)
    at emitReadable_ (_stream_readable.js:407:10)
    at emitReadable (_stream_readable.js:403:5)
    at readableAddChunk (_stream_readable.js:165:9)
    at Socket.Readable.push (_stream_readable.js:127:10)
    at Pipe.onread (net.js:528:21)

@maxogden

This comment has been minimized.

Contributor

maxogden commented Oct 21, 2014

maybe another way to do it that is less unixy but more practical would be to have an opt-out flag rather than an opt-in

@wilzbach

This comment has been minimized.

Member

wilzbach commented Oct 22, 2014

@maxogden sorry. I can't follow you why checking whether there is a tty on stdin is a bad idea?

If you don't pipe anything into your program like

node -p -e "Boolean(process.stdin.isTTY)"

will be true. So what is the matter with this?

if (!process.stdin.isTTY) { 
   process.stdin.pipe(cli.stdin) 
}
@mafintosh

This comment has been minimized.

mafintosh commented Oct 22, 2014

@greenify the problem is that isTTY is false if you run this inside a script that is not attached to your terminal. So stuff like

var proc = require('child_process')
var child = proc.spawn('bionode')

Will open stdin which IMO is a bit magical.

@wilzbach

This comment has been minimized.

Member

wilzbach commented Oct 23, 2014

Ah I see. How about using the "node way"?

process.stdin.setEncoding('utf8');

process.stdin.on('readable', function() {
  process.stdin.pipe(cli.stdin) 
});

process.stdin.on('end', function() {
  cli.stdin.end();
});
@mafintosh

This comment has been minimized.

mafintosh commented Oct 23, 2014

@greenify that could probably work for our node use case but you still have this problem if someone uses bionode inside a bash script or similar that isn't running as a tty.

@bmpvieira bmpvieira added the bug label Apr 4, 2015

@bmpvieira bmpvieira merged commit 562e96e into bionode:master Apr 6, 2015

0 of 2 checks passed

continuous-integration/travis-ci The Travis CI build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details

@bmpvieira bmpvieira self-requested a review Apr 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment