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

* Missing json decoding of response for dataset_versions #28

Closed
wants to merge 4 commits into from

Conversation

billy34
Copy link

@billy34 billy34 commented Nov 19, 2019

Should solve issue #27

@wibeasley
Copy link
Contributor

@billy34, thanks for noticing this in #27 and filing this PR. Because I'm new to this package, I'm reluctant to accept any PRs without an accompanying test. And unfortunately we haven't developed a test suite that's easy for others to add a few tests with a PR of a new feature/correction like yours.

I'd like to add at least a minimal test suite that incorporates this, but realistically that might be a few weeks (see #29).

@pdurbin, @leeper, or anyone else, I don't want to delay @billy34's contribution unnecessarily. Please accept the PR if you'd like.

@pdurbin
Copy link
Member

pdurbin commented Nov 24, 2019

@wibeasley I'm fully supportive of #29 and the issues you linked there: #4 and #22

You are completely sane to want tests in place before merging pull requests. 😄

@billy34 thanks for the pull request! As the brand new maintainer of dataverse-client-r (thanks again!) @wibeasley has inherited a ton of code so please be patient as he comes up to speed and gets comfortable. 😄 If you have any thoughts on adding tests, please let us know!

@wibeasley
Copy link
Contributor

@billy34 , I haven't forgotten about this PR.

Will you please

  1. add yourself to the DESCRIPTION file as a "ctb". Add your ORCID id if you have one.
  2. update NEWS.md to reflect this improvement?

I'm using R Packages for the definition/distinction between the "aut" and "ctb".

@wibeasley
Copy link
Contributor

@billy34, thanks for this. I see that the Travis error are coming from some breaking change in the covr prereq, and nothing to do with your code (or even the package's code).

@kuriwaki
Copy link
Member

kuriwaki commented Dec 28, 2020

@billy34: I fixed unmatched parenthesis in the author listing that were preventing the package from compiling. Anyhow, after that the package_versions don't work. Maybe it's due to changes introduced since your PR. Do these two reprex examples below work for you?

library("dataverse")

## set server internally
Sys.setenv("DATAVERSE_SERVER" = "dataverse.harvard.edu")

# 1. from help page of dataset_versions
monogan <- get_dataverse("monogan")
monogan_data <- dataverse_contents(monogan)
d1 <- get_dataset(monogan_data[[1]])
class(d1)
#> [1] "dataverse_dataset"
dataset_versions(d1)
#> Error in dataset_versions(d1): Not Found (HTTP 404).

# 2. from another simple example on DEMO
d2 <- get_dataset(dataset = "doi:10.70122/FK2/PPIAXE",
                  server = "demo.dataverse.org")
class(d2)
#> [1] "dataverse_dataset"
dataset_versions(d2, server = "demo.dataverse.org")
#> Error in dataset_versions(d2, server = "demo.dataverse.org"): Not Found (HTTP 404).

Created on 2020-12-30 by the reprex package (v0.3.0)

@kuriwaki kuriwaki linked an issue Dec 28, 2020 that may be closed by this pull request
@kuriwaki
Copy link
Member

kuriwaki commented Mar 2, 2021

I don't know JSON as well but it seems 0dc86ca has the same effect as the proposed JSON edit here and is simpler.

@kuriwaki kuriwaki closed this Mar 8, 2021
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.

Error returned by dataset_versions
4 participants