-
Notifications
You must be signed in to change notification settings - Fork 22
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
Fix checks #316
base: main
Are you sure you want to change the base?
Fix checks #316
Conversation
I don't know why this test fail:
I cannot reproduce it in a clean environment |
options = list(select = "id"), | ||
from_publication_date = "2021-01-01", | ||
to_publication_date = "2021-12-31", | ||
options = list(select = "id"), | ||
verbose = TRUE, | ||
pages = c(2, 4:5), | ||
per_page = 10, | ||
verbose = TRUE | ||
per_page = 10 | ||
) | ||
|
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.
Unsure whether this changes behavior or if it just shuffle arguments around (if latter, could we roll it back?)
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.
It just shuffle arguments around to have the same argument order of the previous call.
Still it doesn't fix the failure that I cannnot reproduce in a clean environment
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.
Yes, will need to look into the failing test separately...
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.
Thanks for the PR! I see just a few changes which don't seem related to the failing checks: if they're not relevant, could you roll them back? (or let us know why you feel they're also necessary?)
For what it's worth I also can't seem to replicate this... |
But the test fails also for you? |
As in, the test passes for me, so I can't replicate the failure on github actions! |
The test fails for me. But when I copy paste the code into a clean environment, it passes. |
Could you copy paste the exact message you get? Note that I also pushed a commit to your branch to resolve the warning so you'd want to pull that first |
It's written in #314
|
Could you also paste in the message you get from verbose? When I run the same test and it passes, I see: Requesting url: https://api.openalex.org/works?filter=title.search%3Abibliometric%20analysis%7Cscience%20mapping%2Ccited_by_count%3A%3E50%2Cfrom_publication_date%3A2021-01-01%2Cto_publication_date%3A2021-12-31&select=id
Getting 1 page of results with a total of 191 records...
Requesting url: https://api.openalex.org/works?filter=title.search%3Abibliometric%20analysis%7Cscience%20mapping%2Ccited_by_count%3A%3E50%2Cfrom_publication_date%3A2021-01-01%2Cto_publication_date%3A2021-12-31&select=id
Using basic paging...
Getting 3 pages of results with a total of 30 records... |
Fixes ropensci#314
What do you mean That was the message I get from |
As in, because the So for example, the URLs that I got on my setup are such that: w0_url <- "https://api.openalex.org/works?filter=title.search%3Abibliometric%20analysis%7Cscience%20mapping%2Ccited_by_count%3A%3E50%2Cfrom_publication_date%3A2021-01-01%2Cto_publication_date%3A2021-12-31&select=id"
w24_url <- "https://api.openalex.org/works?filter=title.search%3Abibliometric%20analysis%7Cscience%20mapping%2Ccited_by_count%3A%3E50%2Cfrom_publication_date%3A2021-01-01%2Cto_publication_date%3A2021-12-31&select=id"
identical(
w0_url %>% oa_request() %>% works2df(),
w24_url %>% oa_request() %>% works2df()
)
#> [1] TRUE |
|
Ok so I'm not 100% sure but it seems that these lines from
But those are the same query URLs that I see too! Weirdly, the first URL only fetched 190 records for you, whereas for me that fetches 191 records. Can you confirm that this is the case by running this code? do you get a different length? w0_url <- "https://api.openalex.org/works?filter=title.search%3Abibliometric%20analysis%7Cscience%20mapping%2Ccited_by_count%3A%3E50%2Cfrom_publication_date%3A2021-01-01%2Cto_publication_date%3A2021-12-31&select=id"
w0_url %>% oa_request() %>% length()
#> [1] 191 |
|
Thanks! That's... really weird and I don't have a good intuition. We should probably pluck this out as a separate issue (now #317 - lets catch up there!) Do you have anything you'd like to add to this PR? |
no |
Fixes #314