search

IQSS / dataverse-client-r

R Client for Dataverse Repositories
https://iqss.github.io/dataverse-client-r
61 stars 24 forks source link

Fix handling of fq #36

Closed adam3smith closed 4 years ago

adam3smith commented 4 years ago

Currently fq is completely broken:

  1. It requires start to be set for no reason
  2. It fails because match.arg will always fail since no args are given in the function

This fix does three things:

  1. Test for fq rather than start
  2. get rid of the match.arg -- since fq is super flexible, we can't use that here
  3. enclose the fq query in I to prevent URL encoding (see https://github.com/r-lib/httr/issues/540#issuecomment-426012904 ). Otherwise, httr encodes colons, square brackets and + and e.g. the example for date range search breaks

Please ensure the following before submitting a PR:

adam3smith commented 4 years ago

@wibeasley let me know if you need anything else on this (no particular rush on my end, but FWIW, I've been running this successfully from my fork)

wibeasley commented 4 years ago

thanks @adam3smith for the nudge.I like your fix and love that you included a test. Do you mind adding a few more tests, such as

- [ ] 3+ different search terms (in one url request)

I was planning to add them, but think you're a better fit. I've never used this parameter --and didn't even know what it was when I read your PR.

I am anxious to incorporate your PR and other son, before changes to the master require the PRs to merge the master's changes into the branches before they can be merged back into the master.

adam3smith commented 4 years ago

Will do -- might take me a couple of days, but shouldn't be a problem.

adam3smith commented 4 years ago

@wibeasley working on this -- I've added the zero hits test but I'm actually not quite clear what you're referring to with the two 3+ queries. Am I understanding correctly that those test are general search tests, unrelated to fq? By "in one url request" you just mean a single search for author: King, type: dataset, title: replication ? Not sure what you mean by "by themselves".

(Note that the dataverse API actually allows for multiple fq parameters in a single query, but the package (or maybe even R?) balks at having a parameter repeated. iIf that's OK I'd like to not address that for now since it may require more substantial code changes)

wibeasley commented 4 years ago

@adam3smith, (I'm looking at Advanced Search Example docs now. I've never used this, so feel free to tell me that I not making sense.)

Not sure what you mean by "by themselves".

maybe:

  1. fq = dateSort (which is already in your PR)
  2. fq = publicationDate
  3. fq = description?

Note that the Dataverse API actually allows for multiple fq parameters in a single query, but the package (or maybe even R?) balks at having a parameter repeated.

ahh, I didn't know that, so scratch that 2nd checkbox. Thanks for correcting me.

If that's OK I'd like to not address that for now since it may require more substantial code changes)

Sure. And thanks for adding the zero-hits test.

adam3smith commented 4 years ago

got it, done -- please take a look & let me know if anything else is missing.

The way I understand fq, it's mostly used for the facets available on the left, but there are some additional options such as the dateSort. I have no idea where/whether those are even documented.

wibeasley commented 4 years ago

@adam3smith, looks great. Thanks again.

pdurbin commented 4 years ago

If it helps, "fq" is for "filter query".

The idea is that first you do a query with "q". Under the covers we pass a "*" (wildcard) as "q" when you are browsing Dataverse.

Then you use "fq" to refine or limit or narrow down your original "q". That is to say, there is always a "q" but the "fq" is optional. And as you have observed, you can have as many "fq"s as you want. You can only have one "q".

I hope this makes sense. You can also use boolean operators and such in both "q" and "fq" if you want. And operators like "TO" that @adam3smith is using with dateSort.

wibeasley commented 4 years ago

Just a note in case this becomes a recurring problem. The two Ubuntu builds associated with this PR failed yesterday. I clicked 'Restart Job' on them today and they passed (one of them needed a 3rd try. Because I restarted it, I don't think that failing history is available anymore to insepct.

It had nothing to do with any R code in our repo. Apparently just some server timeout with the gpg key.

I'd been having similar trouble (with similar solutions) on another R package I've been developing today. But according to the Travis status, they've been 100% operational. https://www.traviscistatus.com/

https://travis-ci.org/IQSS/dataverse-client-r/jobs/633831998?

Error thrown yesterday:

Installing R
121.17s$ sudo add-apt-repository -y "ppa:marutter/rrutter3.5"
gpg: keyring `/tmp/tmpad_p2bj9/secring.gpg' created
gpg: keyring `/tmp/tmpad_p2bj9/pubring.gpg' created
gpg: requesting key B04C661B from hkp server keyserver.ubuntu.com
Error: retrieving gpg key timed out.
gpg: keyserver timed out
gpg: keyserver receive failed: keyserver error
The command "sudo add-apt-repository -y "ppa:marutter/rrutter3.5"" failed and exited with 1 during .

Error thrown today

Installing R
1.73s$ sudo add-apt-repository -y "ppa:marutter/rrutter3.5"
gpg: keyring `/tmp/tmp3vrq9j3r/secring.gpg' created
gpg: keyring `/tmp/tmp3vrq9j3r/pubring.gpg' created
gpg: requesting key B04C661B from hkp server keyserver.ubuntu.com
gpg: /tmp/tmp3vrq9j3r/trustdb.gpg: trustdb created
gpg: key B04C661B: public key "Launchpad PPA for marutter" imported
gpg: Total number processed: 1
gpg:               imported: 1  (RSA: 1)
OK
66.78s$ sudo add-apt-repository -y "ppa:marutter/c2d4u3.5"
Error: retrieving gpg key timed out.
gpg: keyring `/tmp/tmprn4clv38/secring.gpg' created
gpg: keyring `/tmp/tmprn4clv38/pubring.gpg' created
gpg: requesting key B04C661B from hkp server keyserver.ubuntu.com
gpg: /tmp/tmprn4clv38/trustdb.gpg: trustdb created
gpg: key B04C661B: public key "Launchpad PPA for marutter" imported
gpg: Total number processed: 1
gpg:               imported: 1  (RSA: 1)
OK
The command "sudo add-apt-repository -y "ppa:marutter/c2d4u3.5"" failed and exited with 1 during .
Your build has been stopped.
pdurbin commented 4 years ago

@wibeasley @adam3smith when one of you has a minute can you please create an issue over at https://github.com/IQSS/dataverse-jenkins/issues to add a job for dataverse-client-r to https://jenkins.dataverse.org ? I love, love, love Travis but it would be nice to have another view into if tests are passing or not. Belt and suspenders. 😄