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

CLI #10

Merged
merged 6 commits into from Aug 22, 2017
Merged

CLI #10

merged 6 commits into from Aug 22, 2017

Conversation

Colelyman
Copy link
Member

This is a CLI that is heavily based on the CLI from bionode-fasta.

This PR is in response to #8.

I'm not sure if test cases are necessary for a CLI, but I can make them if needed.

Copy link
Member

@thejmazz thejmazz left a comment

Choose a reason for hiding this comment

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

thanks @Colelyman!

In addition to my comments, I would also suggest switching usage of var to const and let; if we are going to include package-lock.json which is an npm v5 feature, I think it makes sense to use recent syntax as well (at least for new code).

cli.js Outdated
#!/usr/bin/env node
var fs = require('fs')
var minimist = require('minimist')
var fastq = require('./index')
Copy link
Member

Choose a reason for hiding this comment

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

include .js extension

Copy link
Member

Choose a reason for hiding this comment

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

style: blank line after #!/usr/bin/env node

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 these are fixed, thanks for pointing them out!

cli.js Outdated
})

fq.on('error', function (err) {
console.log(
Copy link
Member

Choose a reason for hiding this comment

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

one line console.log('There was an error:\n', err)

@bmpvieira @tiagofilipe12 do we have an issue for standardized error messages across modules?

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 fixed

cli.js Outdated

process.stdin.setEncoding('utf8')

fq.on('data', function (data) {
Copy link
Member

@thejmazz thejmazz Aug 19, 2017

Choose a reason for hiding this comment

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

using .on('data') puts a stream into streams 1 mode - it is preferred to use a pipe instead. It would look something like

const through = require('through2')

fq.pipe(through.obj(function (data, enc, next) {
  this.push(JSON.stringify(data) + '\n')
  next()
  // alternatively, next(null, JSON.stringify(data) + '\n')
})
  .pipe(writeStream)

There is also ndjson.stringify which I believe does the same thing. (and it handles linux vs windows EOL)

So: use ndjson module.

Copy link
Member Author

@Colelyman Colelyman Aug 19, 2017

Choose a reason for hiding this comment

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

@thejmazz I am very new to nodejs streams, so I probably just don't understand the concept behind pipes. But, would using the code that you referenced above require the parser to be refactored? From what I understand, fastq.read(argv._[0]) returns an events.EventEmitter() object, so can an events.EventEmitter() be piped?

Perhaps this is the function of the ndjson module and I just don't know it.

Copy link
Member Author

Choose a reason for hiding this comment

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

From what I can see it looks like the Refactoring branch might be a solution?

Copy link
Member

Choose a reason for hiding this comment

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

@thejmazz This is related with the Refactoring branch I made. Check the lib here. I also added checks for if lines are corrupt or malformed but this branch has no CLI and needs to be finished. I think it just needs to export module instead of running it as I did for test purposes (check this line).

Copy link
Member

Choose a reason for hiding this comment

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

Ahh yes @Colelyman my bad - I thought this module was a stream. It does not make sense to pipe an event emitter. The CLI from this PR looks like it will work with the changes in refactoring branch - I suggest we merge this in now, finish refactoring (which I think is just some cleaning up and making sure tests pass), and as part of that can implement fs.createReadStream(input).pipe(fastq) in the CLI - which is what it looks like @tiagofilipe12 was using to test refactoring branch.

Copy link
Member

Choose a reason for hiding this comment

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

👍

@bmpvieira
Copy link
Member

Thanks @Colelyman for the contribution! 🎉 Since this has been reviewed and approved you can go ahead and click the merge button yourself if you want. ;-)

@Colelyman
Copy link
Member Author

Awesome, thanks @bmpvieira!

@Colelyman Colelyman merged commit 300ceed into bionode:master Aug 22, 2017
@thejmazz thejmazz mentioned this pull request Aug 22, 2017
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants