Skip to content

dont open stdin by default#15

Merged
bmpvieira merged 1 commit intobionode:masterfrom
max-mapper:stdin
Apr 6, 2015
Merged

dont open stdin by default#15
bmpvieira merged 1 commit intobionode:masterfrom
max-mapper:stdin

Conversation

@max-mapper
Copy link
Contributor

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

@max-mapper
Copy link
Contributor Author

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)

@max-mapper
Copy link
Contributor Author

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
Copy link
Member

@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
Copy link

@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
Copy link
Member

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
Copy link

@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
@bmpvieira bmpvieira self-requested a review April 6, 2017 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants