Skip to content
Please note that GitHub no longer supports your web browser.

We recommend upgrading to the latest Google Chrome or Firefox.

Learn more
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 handling of fq #36

Merged
merged 4 commits into from Jan 7, 2020
Merged

Fix handling of fq #36

merged 4 commits into from Jan 7, 2020

Conversation

@adam3smith
Copy link
Contributor

adam3smith commented Dec 21, 2019

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 r-lib/httr#540 (comment) ). 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:

  • if suggesting code changes or improvements, open an issue first
  • for all but trivial changes (e.g., typo fixes), add your name to DESCRIPTION
  • for all but trivial changes (e.g., typo fixes), documentation your change in NEWS.md with a parenthetical reference to the issue number being addressed
  • if changing documentation, edit files in /R not /man and run devtools::document() to update documentation
  • add code or new test files to /tests for any new functionality or bug fix
  • make sure R CMD check runs without error before submitting the PR
adam3smith added 2 commits Dec 21, 2019
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 r-lib/httr#540 (comment) ). Otherwise, httr encodes colons, square brackets and + and e.g. the [example for date range search](http://guides.dataverse.org/en/latest/api/search.html#date-range-search-example) breaks
@adam3smith adam3smith mentioned this pull request Dec 21, 2019
1 of 3 tasks complete
@adam3smith

This comment has been minimized.

Copy link
Contributor Author

adam3smith commented Jan 6, 2020

@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

This comment has been minimized.

Copy link
Contributor

wibeasley commented Jan 6, 2020

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 (by themselves)
  • an fq that returns zero hits

- [ ] 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

This comment has been minimized.

Copy link
Contributor Author

adam3smith commented Jan 6, 2020

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

@adam3smith

This comment has been minimized.

Copy link
Contributor Author

adam3smith commented Jan 7, 2020

@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

This comment has been minimized.

Copy link
Contributor

wibeasley commented Jan 7, 2020

@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

This comment has been minimized.

Copy link
Contributor Author

adam3smith commented Jan 7, 2020

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 wibeasley merged commit 012058e into IQSS:master Jan 7, 2020
3 checks passed
3 checks passed
codecov/patch 100% of diff hit (target 0%)
Details
codecov/project 12.22% (+12.22%) compared to bac89f4
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@wibeasley

This comment has been minimized.

Copy link
Contributor

wibeasley commented Jan 7, 2020

@adam3smith, looks great. Thanks again.

@pdurbin

This comment has been minimized.

Copy link
Member

pdurbin commented Jan 7, 2020

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.

@adam3smith adam3smith deleted the adam3smith:patch-1 branch Jan 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.