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

Do the db filter at lib level #33

Merged
merged 1 commit into from
Aug 12, 2017

Conversation

Istar-Eldritch
Copy link
Member

@Istar-Eldritch Istar-Eldritch commented Apr 30, 2017

This PR references #20

A new module valid-dbs is defined in /lib/valid-dbs.js to handle the logic of the valid dbs.
It exports:

  • dbs An object that contains the valid db options as keys and descriptions.
  • InvalidDbError Custom Error to throw in functions that require a valid dbs
  • printDbs A function that pretty prints the dbs following this format key (description) and separated by new lines.

This also modifies the search function in main ncbi lib file to throw an exception is the db is invalid.
Minor modifications to the cli file to use the valid-dbs module to handle the logic when no valid db is provided.
Updates the package.json file so all the test files are run in the CI and the coverage reporter uses all the test results.

@bmpvieira bmpvieira self-requested a review April 30, 2017 14:01
@bmpvieira bmpvieira added this to the 2.1.0 milestone Apr 30, 2017
@Istar-Eldritch
Copy link
Member Author

I am adding now the tests to pass the coverage requirements.

Copy link
Member

@bmpvieira bmpvieira left a comment

Choose a reason for hiding this comment

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

Thank you! Overall looks good, but could you maybe squash some of the commits? Like "Fix indentation", linter and "Add tests to valid-dbs" that has duplicated message.

@Istar-Eldritch
Copy link
Member Author

I would squash all of them if that's ok. Do you agree?

@bmpvieira
Copy link
Member

Sure, go for it!

Istar-Eldritch added a commit to Istar-Eldritch/bionode-ncbi that referenced this pull request May 5, 2017
Do the db filter at lib level

Add tests to valid-dbs

Fix identation

Istanbul should use all the test files for coverage
@Istar-Eldritch
Copy link
Member Author

Squashed :)

Copy link
Member

@tiagofilipe12 tiagofilipe12 left a comment

Choose a reason for hiding this comment

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

Looks good but there is just one small change.

cli.js Outdated
pubmed, pubmedhealth, snp, sparcle, sra, structure, \
taxonomy, toolkit, toolkitall, toolkitbook, toolkitbookgh, unigene`
)
.example('databases available', validDbs.printDbs())
Copy link
Member

Choose a reason for hiding this comment

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

Please add .choices('db', validDbs.printDbs()) before this line. Otherwise, you don't get an error if you choose a wrong database.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, you would get an error bubbling up from the library itself, but it won't show the valid options. I added Object.keys(validDbs.dbs) as it looks cleaner without the descriptions and is actually resembling the previous behavior.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you. I have tested it and it returns the previous behavior.

Do the db filter at lib level

Add tests to valid-dbs

Fix identation

Istanbul should use all the test files for coverage

Show choices when provided db in cli is invalid
@Istar-Eldritch
Copy link
Member Author

I added database names to the cli choices as requested 😉 . I forgot to add a comment to notify you, please take a look and let me know if we need to change anything else.

@tiagofilipe12
Copy link
Member

Thanks for your contribution. Now, it is easy to understand the options if we fail one database type, however we should address this issue in the future: #38.

@Istar-Eldritch
Copy link
Member Author

Istar-Eldritch commented Jun 12, 2017

Good point. I'll create a new PR for it 😉
Should I merge this one or should I do a rebase instead?

@Istar-Eldritch
Copy link
Member Author

Ping

@thejmazz
Copy link
Member

ping @tiagofilipe12 @bmpvieira

@tiagofilipe12
Copy link
Member

Sorry for the delay in my response. I think you are good to merge @Istar-Eldritch. Maybe the thing with #38 could be handled in another PR.

@Istar-Eldritch
Copy link
Member Author

Sure

@Istar-Eldritch Istar-Eldritch merged commit 8924642 into bionode:master Aug 12, 2017
@bmpvieira bmpvieira mentioned this pull request Aug 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants