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

epmc_search: populate the hit_count attribute even with zero results #58

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

arvi1000
Copy link

This addresses issue 56, and now sets the hit_count attribute even when no results found. Note that we can't return a NULL result anymore, since NULL can't have attributes, so instead we return an empty tibble

@arvi1000
Copy link
Author

arvi1000 commented Jun 13, 2024

I see failing tests, but it appears to failing on this line, which asserts that the following should return 50 rows:

 a <- epmc_db("12368864", db = "uniprot", limit = 50)

However in the current cran version, it seems to only return 10 rows:

> library(europepmc)
> epmc_db("12368864", db = "uniprot", limit = 50)
10 records found. Returning 10

So perhaps this test needs an update and the change in the PR is not the issue? I welcome a reviewer's comments, thanks

@arvi1000
Copy link
Author

Looking at the API directly, 10 does seem to be the current correct number of hits returned. Using source = 'med' as that's the default and nothing explicit is passed in the test line

https://www.ebi.ac.uk/europepmc/webservices/rest/med/12368864/databaseLinks?database=UNIPROT&page=1&pageSize=50&format=json

I'm happy to update the test as part of this PR if that suits (e.g. limit = 7 and then test that 7 records come back)

@arvi1000
Copy link
Author

arvi1000 commented Jul 1, 2024

I changed the test mentioned in the comment above. I also updated the test relating to zero result queries, since they now return blank tibbles with hit_count attr = 0 instead of NULL. So now all tests pass

@arvi1000
Copy link
Author

arvi1000 commented Aug 20, 2024

@njahn82 any chance to tale a look at this PR? Addresses issue 56 (#56)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant