-
Notifications
You must be signed in to change notification settings - Fork 20
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
Conversation
I am adding now the tests to pass the coverage requirements. |
There was a problem hiding this 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.
I would squash all of them if that's ok. Do you agree? |
Sure, go for it! |
Do the db filter at lib level Add tests to valid-dbs Fix identation Istanbul should use all the test files for coverage
Squashed :) |
There was a problem hiding this 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()) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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. |
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. |
Good point. I'll create a new PR for it 😉 |
Ping |
ping @tiagofilipe12 @bmpvieira |
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. |
Sure |
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 dbsprintDbs
A function that pretty prints the dbs following this formatkey (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.