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
Member

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 added this to In Progress in Bionode Project Board Apr 30, 2017

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

@bmpvieira bmpvieira added the feature label Apr 30, 2017

@bmpvieira bmpvieira added this to the 2.1.0 milestone Apr 30, 2017

@Istar-Eldritch

This comment has been minimized.

Member

Istar-Eldritch commented Apr 30, 2017

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

@bmpvieira

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

This comment has been minimized.

Member

Istar-Eldritch commented May 4, 2017

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

@bmpvieira

This comment has been minimized.

Member

bmpvieira commented May 4, 2017

Sure, go for it!

Istar-Eldritch added a commit to Istar-Eldritch/bionode-ncbi that referenced this pull request May 5, 2017

(bionode#33) [Feature] Do the ncbi valid db checks at lib level
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

This comment has been minimized.

Member

Istar-Eldritch commented May 5, 2017

Squashed :)

@tiagofilipe12

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())

This comment has been minimized.

@tiagofilipe12

tiagofilipe12 Jun 1, 2017

Member

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

This comment has been minimized.

@Istar-Eldritch

Istar-Eldritch Jun 2, 2017

Member

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.

This comment has been minimized.

@tiagofilipe12

tiagofilipe12 Jun 7, 2017

Member

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

@bmpvieira bmpvieira added the mozsprint label Jun 2, 2017

[Feature] Do the ncbi valid db checks at lib level (#33)
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

This comment has been minimized.

Member

Istar-Eldritch commented Jun 3, 2017

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

This comment has been minimized.

Member

tiagofilipe12 commented Jun 12, 2017

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

Istar-Eldritch commented Aug 11, 2017

Ping

@thejmazz

This comment has been minimized.

Member

thejmazz commented Aug 12, 2017

@tiagofilipe12

This comment has been minimized.

Member

tiagofilipe12 commented Aug 12, 2017

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

This comment has been minimized.

Member

Istar-Eldritch commented Aug 12, 2017

Sure

@Istar-Eldritch Istar-Eldritch merged commit 8924642 into bionode:master Aug 12, 2017

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+1.06%) to 81.572%
Details
security/snyk No new vulnerabilities
Details

@bmpvieira bmpvieira referenced this pull request Aug 22, 2017

Closed

geo is not searchable #20

@bmpvieira bmpvieira moved this from In Progress to Done in Bionode Project Board Aug 22, 2017

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