Closed maxheld83 closed 1 year ago
Thanks @maxheld83
In general I prefer PR's that are focused and smaller rather than less focused and larger. So I'd prefer unrelated topics in separate PRs
get_email
and get_md_plus_token
?rcrossref_header()
, but what is the use case for exporting it?verbose=TRUE
, or crul::set_verbose()
(sets verbose globally in the R session). I'd probably compare headers in tests using vcr
, e.g., without_mdplus <- vcr::use_cassette("compare_response_headers_without_mdplus", {
cr_abstract('10.1109/TASC.2010.2088091')
})
with_mdplus <- vcr::use_cassette("compare_response_headers_with_mdplus", {
cr_abstract('10.1109/TASC.2010.2088091') # however that's triggered
})
# load the stored on disk http requests
w_o <- yaml::yaml.load_file(without_mdplus$file())
with <- yaml::yaml.load_file(with_mdplus$file())
# get to headers like
w_o$http_interactions[[1]]$response$headers
with$http_interactions[[1]]$response$headers
We're using now using a metadata plus (mdplus) subscription @subugoe and @njahn82 and myself wanted to contribute support for mdplus back upstream.
This is still a very early draft with lots (test, documentation) to be added; just wanted to log this here to show that it's being worked on.
I also have a couple of questions/ideas associated with mdplus support I'd love your feedback on @sckott and @njahn82 before I proceed. If you can, please let me know which of these (if any) you'd find worthwhile and would accept in the same (or separate?) PRs:
crossref_email
and (tba.)crossref_plus
to all uppercase as per Google's shell styleguide. This is just cosmetic, but I wonder whether it might help people recognise that these are "just" normal environment variables. (The lowercase env var would have to continue to work so as not to break stuff).get_email()
andget_md_plus_token()
together (maybe with belowrcrossref_header()
, and link from theREADME.md
and other places to that documentation, so "politeness" and md-plus "auth" are in one place..Renviron
s safe, link to related resources (i.e. using secrets in CI or OS keychain facilities) as well as resources on user/project.Renviron
s. Maybe add a sentence that.Renviron
s are really just a way to add env vars to R processes. The mdplus crossref token is somewhat sensitive; at least it should not be published.[ ] I noticed that there are a bunch of calls like the below in the codebase: https://github.com/ropensci/rcrossref/blob/4d38a0aa2bb2fa2a27ee31658a92558cc62b3a13/R/cr_citation_count_async.R#L27-L30
Which now all need
get_md_plus_token()
as well. While we're at it, what's the best way to factor this out? Maybe add and document anrcrossref_header()
with these defaults and run that? Or is there a better, more idiomatic way to declare these defaults with crul? I've only used httr so far, so apologies for my ignorance here.NEWS.md
.README.Rmd
.[ ] add tests. I'd like to test that when
crossref_plus
is available, we're actually getting the improved service, though this is an integration test, not a unit test, I guess. (theropensci/rcrossref
repo could store the token as a secret, and the test would be skipped whenever that is unavailable -- that's what I'm currently doing in https://github.com/subugoe/metacheck/commit/2efc2da05d94c6b806f33dcc076f78c996fc64db). I'd like to test this by snapshotting (or parsing and comparing) the response headers with and without mdplus. Among other things (?) at least the rate limits should be reliably higher for mdplus. Is this a good way to test? Again, I'm ignorant about the crul/vcr/webmockr etc HTTP testing ecosystem, so I'd appreciate any pointers.If comparing response headers is an acceptable test, is there a good way to capture it from, say
cr_abstract('10.1109/TASC.2010.2088091', verbose = TRUE)
? I couldn't get ahold of the response header withcapture.output()
, try as I might, but I'm probably missing something.(I also tried benchmarking the clock time with/without mdplus, but that's too flaky as I should've expected).