search

ropensci / software-review

rOpenSci Software Peer Review.
295 stars 104 forks source link

EDIutils #498

Closed clnsmth closed 2 years ago

clnsmth commented 2 years ago

Date accepted: 2022-03-29 Reviewers:

Submitting Author Name: Colin Smith Submitting Author Github Handle: !--author1-->@clnsmth<!--end-author1-- Repository: https://github.com/EDIorg/EDIutils Version submitted: Submission type: Standard Editor: !--editor-->@adamhsparks<!--end-editor-- Reviewers: @bozaah, @laijasmine

Due date for @bozaah: 2022-02-25 Due date for @laijasmine: 2022-02-25

Archive: TBD Version accepted: TBD Language: en

Package: EDIutils
Title: An API Client for the Environmental Data Initiative Repository
Version: 0.0.0.9000
Authors@R: 
    c(person("Colin", "Smith", email = "colin.smith@wisc.edu", role = c("aut", "cre"), comment = "https://orcid.org/0000-0003-2261-9931"),
    person("Corinna", "Gries", role = "ctb", comment = "0000-0002-9091-6543"))
Description: A client for the Environmental Data Initiative repository REST API. The EDI data repository <https://portal.edirepository.org/nis/home.jsp> is for publication and reuse of ecological data with emphasis on metadata accuracy and completeness. It is built upon the PASTA+ software stack <https://pastaplus-core.readthedocs.io/en/latest/index.html#> and was developed in collaboration with the US LTER Network <https://lternet.edu/>. EDIutils includes functions to search and access existing data, evaluate and upload new data, and assist other data management tasks common to repository users.
Imports: 
    curl,
    httr,
    xml2
License: MIT + file LICENSE
Encoding: UTF-8
LazyData: true
RoxygenNote: 7.1.1
Suggests: 
    knitr,
    readr,
    rmarkdown,
    roxygen2,
    testthat
URL: https://github.com/EDIorg/EDIutils, https://ediorg.github.io/EDIutils/
BugReports: https://github.com/EDIorg/EDIutils/issues
VignetteBuilder: knitr
Language: en-US

Scope

- [x ] data retrieval
- [ ] data extraction
- [ ] data munging
- [ x] data deposition
- [ ] workflow automation
- [ ] version control
- [ ] citation management and bibliometrics
- [ ] scientific software wrappers
- [ ] field and lab reproducibility tools
- [ ] database software bindings
- [ ] geospatial data
- [ ] text analysis

The package is an API client for the Environmental Data Initiative data repository, which serves the ecological and environmental science and monitoring domains.

The target audiences are: 1.) Data authors who publish to the repository. 2.) Data users who retrieve data from the repository.

There is no overlap.

Yes

All checks pass

Technical checks

Confirm each of the following by checking the box.

This package:

Publication options

MEE Options - [ ] The package is novel and will be of interest to the broad readership of the journal. - [ ] The manuscript describing the package is no longer than 3000 words. - [ ] You intend to archive the code for the package in a long-term repository which meets the requirements of the journal (see [MEE's Policy on Publishing Code](http://besjournals.onlinelibrary.wiley.com/hub/journal/10.1111/(ISSN)2041-210X/journal-resources/policy-on-publishing-code.html)) - (*Scope: Do consider MEE's [Aims and Scope](http://besjournals.onlinelibrary.wiley.com/hub/journal/10.1111/(ISSN)2041-210X/aims-and-scope/read-full-aims-and-scope.html) for your manuscript. We make no guarantee that your manuscript will be within MEE scope.*) - (*Although not required, we strongly recommend having a full manuscript prepared when you submit here.*) - (*Please do not submit your package separately to Methods in Ecology and Evolution*)

Code of conduct

ropensci-review-bot commented 2 years ago

Thanks for submitting to rOpenSci, our editors and @ropensci-review-bot will reply soon. Type @ropensci-review-bot help for help.

ropensci-review-bot commented 2 years ago

:rocket:

Editor check started

:wave:

ropensci-review-bot commented 2 years ago

Checks for EDIutils (v0.0.0.9000)

git hash: 70932fd3

Important: All failing checks above must be addressed prior to proceeding

Package License: MIT + file LICENSE

1. Statistical Properties

This package features some noteworthy statistical properties which may need to be clarified by a handling editor prior to progressing.

Details of statistical properties (click to open)

The package has: - code in R (100% in 72 files) and - 1 authors - 5 vignettes - no internal data file - 3 imported packages - 71 exported functions (median 9 lines of code) - 100 non-exported functions in R (median 11 lines of code) --- Statistical properties of package structure as distributional percentiles in relation to all current CRAN packages The following terminology is used: - `loc` = "Lines of Code" - `fn` = "function" - `exp`/`not_exp` = exported / not exported The final measure (`fn_call_network_size`) is the total number of calls between functions (in R), or more abstract relationships between code objects in other languages. Values are flagged as "noteworthy" when they lie in the upper or lower 5th percentile. |measure | value| percentile|noteworthy | |:------------------------|-----:|----------:|:----------| |files_R | 72| 98.1| | |files_vignettes | 5| 96.9| | |files_tests | 72| 99.6| | |loc_R | 1178| 72.3| | |loc_vignettes | 383| 71.2| | |loc_tests | 659| 80.2| | |num_vignettes | 5| 97.9|TRUE | |n_fns_r | 171| 87.3| | |n_fns_r_exported | 71| 92.3| | |n_fns_r_not_exported | 100| 83.8| | |n_fns_per_file_r | 1| 19.0| | |num_params_per_fn | 2| 11.9| | |loc_per_fn_r | 10| 28.4| | |loc_per_fn_r_exp | 9| 19.2| | |loc_per_fn_r_not_exp | 11| 35.4| | |rel_whitespace_R | 9| 53.4| | |rel_whitespace_vignettes | 38| 76.3| | |rel_whitespace_tests | 13| 68.9| | |doclines_per_fn_exp | 38| 47.0| | |doclines_per_fn_not_exp | 0| 0.0|TRUE | |fn_call_network_size | 230| 90.0| | ---

1a. Network visualisation

Click to see the interactive network visualisation of calls between objects in package

2. goodpractice and other checks

Details of goodpractice and other checks (click to open)

#### 3a. Continuous Integration Badges [![R-CMD-check](https://github.com/EDIorg/EDIutils/workflows/R-CMD-check/badge.svg)](https://github.com/EDIorg/EDIutils/actions) **GitHub Workflow Results** |name |conclusion |sha |date | |:--------------------------|:----------|:------|:----------| |pages build and deployment |success |70932f |2022-01-11 | |pkgdown |success |70932f |2022-01-11 | |R-CMD-check | |70932f |2022-01-11 | |test-coverage |success |70932f |2022-01-11 | --- #### 3b. `goodpractice` results #### `R CMD check` with [rcmdcheck](https://r-lib.github.io/rcmdcheck/) rcmdcheck found no errors, warnings, or notes #### Test coverage with [covr](https://covr.r-lib.org/) Package coverage: 55.63 The following files are not completely covered by tests: file | coverage --- | --- R/check_status_create.R | 0% R/check_status_evaluate.R | 0% R/check_status_update.R | 0% R/create_data_package.R | 0% R/create_event_subscription.R | 0% R/create_journal_citation.R | 0% R/create_reservation.R | 0% R/delete_event_subscription.R | 0% R/delete_journal_citation.R | 0% R/delete_reservation.R | 0% R/evaluate_data_package.R | 0% R/execute_event_subscription.R | 0% R/get_audit_count.R | 0% R/get_audit_record.R | 0% R/get_audit_report.R | 0% R/get_event_subscription.R | 0% R/is_authorized.R | 0% R/login.R | 44% R/logout.R | 0% R/query_event_subscriptions.R | 0% R/read_data_package_error.R | 0% R/read_evaluate_report_summary.R | 0% R/read_evaluate_report.R | 0% R/update_data_package.R | 0% #### Cyclocomplexity with [cyclocomp](https://github.com/MangoTheCat/cyclocomp) No functions have cyclocomplexity >= 15 #### Static code analyses with [lintr](https://github.com/jimhester/lintr) [lintr](https://github.com/jimhester/lintr) found the following 26 potential issues: message | number of times --- | --- Lines should not be more than 80 characters. | 26

Package Versions

|package |version | |:--------|:---------| |pkgstats |0.0.3.82 | |pkgcheck |0.0.2.205 |

Editor-in-Chief Instructions:

Processing may not proceed until the items marked with :heavy_multiplication_x: have been resolved.

ldecicco-USGS commented 2 years ago

Hi @clnsmth ! Thanks for the submission. The scope of the package looks like a good fit for rOpenSci. I'm going to assign an editor.

As for the test coverage, have a look here: https://books.ropensci.org/http-testing/ And see if there are some techniques you could use to bump up the coverage.

Thanks for the submission!

@ropensci-review-bot assign @adamhsparks as editor

ldecicco-USGS commented 2 years ago

@ropensci-review-bot assign @adamhsparks as editor

ropensci-review-bot commented 2 years ago

Assigned! @adamhsparks is now the editor

clnsmth commented 2 years ago

Hi @ldecicco-USGS and @adamhsparks ! Thanks for the http-testing resource. I'll send an update once the requested changes are implemented.

adamhsparks commented 2 years ago

Thanks, @clnsmth, just ping me when you're ready and I'll start my editor checks and look for reviewers when everything looks good here

clnsmth commented 2 years ago

Hi @adamhsparks @ldecicco-USGS,

I've implement rOpenSci http testing recommendations and coverage is now > 75%.

EDIutils:

Thanks again for this helpful advice and please let me know if I've missed anything.

adamhsparks commented 2 years ago

@ropensci-review-bot check package

ropensci-review-bot commented 2 years ago

Thanks, about to send the query.

ropensci-review-bot commented 2 years ago

:rocket:

Editor check started

:wave:

ropensci-review-bot commented 2 years ago

Checks for EDIutils (v0.0.0.9001)

git hash: d80e7bbb

Package License: MIT + file LICENSE

1. Statistical Properties

This package features some noteworthy statistical properties which may need to be clarified by a handling editor prior to progressing.

Details of statistical properties (click to open)

The package has: - code in R (100% in 72 files) and - 1 authors - 5 vignettes - no internal data file - 3 imported packages - 71 exported functions (median 9 lines of code) - 100 non-exported functions in R (median 11 lines of code) --- Statistical properties of package structure as distributional percentiles in relation to all current CRAN packages The following terminology is used: - `loc` = "Lines of Code" - `fn` = "function" - `exp`/`not_exp` = exported / not exported The final measure (`fn_call_network_size`) is the total number of calls between functions (in R), or more abstract relationships between code objects in other languages. Values are flagged as "noteworthy" when they lie in the upper or lower 5th percentile. |measure | value| percentile|noteworthy | |:------------------------|-----:|----------:|:----------| |files_R | 72| 98.1| | |files_vignettes | 5| 96.9| | |files_tests | 146| 99.9| | |loc_R | 1179| 72.4| | |loc_vignettes | 383| 71.2| | |loc_tests | 5333| 98.4|TRUE | |num_vignettes | 5| 97.9|TRUE | |n_fns_r | 171| 87.3| | |n_fns_r_exported | 71| 92.3| | |n_fns_r_not_exported | 100| 83.8| | |n_fns_per_file_r | 1| 19.0| | |num_params_per_fn | 2| 11.9| | |loc_per_fn_r | 10| 28.4| | |loc_per_fn_r_exp | 9| 19.2| | |loc_per_fn_r_not_exp | 11| 35.4| | |rel_whitespace_R | 9| 53.6| | |rel_whitespace_vignettes | 38| 76.3| | |rel_whitespace_tests | 2| 73.5| | |doclines_per_fn_exp | 47| 59.4| | |doclines_per_fn_not_exp | 0| 0.0|TRUE | |fn_call_network_size | 230| 90.0| | ---

1a. Network visualisation

Click to see the interactive network visualisation of calls between objects in package

2. goodpractice and other checks

Details of goodpractice and other checks (click to open)

#### 3a. Continuous Integration Badges [![R-CMD-check](https://github.com/EDIorg/EDIutils/workflows/R-CMD-check/badge.svg)](https://github.com/EDIorg/EDIutils/actions) **GitHub Workflow Results** |name |conclusion |sha |date | |:--------------------------|:----------|:------|:----------| |pages build and deployment |success |ea61c2 |2022-01-21 | |pkgdown |success |d80e7b |2022-01-21 | |R-CMD-check |success |971e12 |2022-01-21 | |R-CMD-check-real-requests |success |d80e7b |2022-01-21 | |test-coverage |cancelled |6114a2 |2022-01-21 | --- #### 3b. `goodpractice` results #### `R CMD check` with [rcmdcheck](https://r-lib.github.io/rcmdcheck/) rcmdcheck found no errors, warnings, or notes #### Test coverage with [covr](https://covr.r-lib.org/) Package coverage: 77.51 #### Cyclocomplexity with [cyclocomp](https://github.com/MangoTheCat/cyclocomp) No functions have cyclocomplexity >= 15 #### Static code analyses with [lintr](https://github.com/jimhester/lintr) [lintr](https://github.com/jimhester/lintr) found the following 3 potential issues: message | number of times --- | --- Lines should not be more than 80 characters. | 3

Package Versions

|package |version | |:--------|:---------| |pkgstats |0.0.3.86 | |pkgcheck |0.0.2.207 |

Editor-in-Chief Instructions:

This package is in top shape and may be passed on to a handling editor

adamhsparks commented 2 years ago

Editor checks:

Editor comments

Thanks for addressing the issues that were flagged by @ldecicco-USGS. This looks great, thanks @clnsmth. I'll start looking for reviewers now.

adamhsparks commented 2 years ago

@ropensci-review-bot seeking reviewers

ropensci-review-bot commented 2 years ago

Please add this badge to the README of your package repository:

[![Status at rOpenSci Software Peer Review](https://badges.ropensci.org/498_status.svg)](https://github.com/ropensci/software-review/issues/498)

Furthermore, if your package does not have a NEWS.md file yet, please create one to capture the changes made during the review process. See https://devguide.ropensci.org/releasing.html#news

adamhsparks commented 2 years ago

@ropensci-review-bot add @bozaah to reviewers

ropensci-review-bot commented 2 years ago

@bozaah added to the reviewers list. Review due date is 2022-02-25. Thanks @bozaah for accepting to review! Please refer to our reviewer guide.

ropensci-review-bot commented 2 years ago

@bozaah: If you haven't done so, please fill this form for us to update our reviewers records.

adamhsparks commented 2 years ago

@ropensci-review-bot add @laijasmine to reviewers

ropensci-review-bot commented 2 years ago

@laijasmine added to the reviewers list. Review due date is 2022-02-25. Thanks @laijasmine for accepting to review! Please refer to our reviewer guide.

ropensci-review-bot commented 2 years ago

@laijasmine: If you haven't done so, please fill this form for us to update our reviewers records.

laijasmine commented 2 years ago

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

Documentation

The package includes all the following forms of documentation:

Functionality

Estimated hours spent reviewing: 8

Review Comments

Thanks for the opportunity to be involved in this! It was interesting to see how other data repositories deal with data upload. I also wanted to preface this by saying my experience is heavily influenced by data repository access and submission package, dataone and working on editing EML using the EML package. The workflow there is very similar but different in some elements so some of the things I might not be applicable to your situation!

Vignettes

Search and Access

Many users are probably unfamiliar with xml and how to navigate it and there is a learning curve involved so a vignette on the files would be helpful. They might be more comfortable with working with dataframes instead. We don't need to go into detail but it might be helpful to point them to packages like XML to help convert the content to a preferred format. Maybe adding a couple lines or point them to retrieve downloads will be helpful.

Evaluate and upload

Upload a data package I struggled the longest here. Maybe because I was just trying to write the least amount of information possible to just upload the file. I had a hard time understanding what the errors were in the results of read_evaluate_report. I went back and forth between the ezEML form and R trying to figure out what I needed. There seems to be more requirements to upload using EDIutils compared to the Check Metadata in ezEML. For example I tried to avoid uploading a data table because it wasn't marked as required in ezEML but it was required to upload using EDIutils.

Report I also found the output from read_evaluate_reportquite long and hard to read if I'm viewing it as a message in the console. We might want to suggest users to use writeLines(report, "report.txt") instead so they can cmd + f the file and keep track of their fixes. Users might prefer if there is a way to only show the warnings and errors so there is less to look through when trying to improve their metadata.

Update a data package Following the example in the vignette I wasn't able to update my package because I keep getting the error:

Error: Attempting to update a data package to revision '1' but an equal or higher revision ('1') already exists in PASTA: edi.757.1

I realized the packageId slot inside the eml file also needs to be updated. We might need to be more explicit in the vignette that the identifier needs to be updated. I used the following EML package here:

my_eml <- EML::read_eml("edi.757.1.xml")
my_eml$dataset$title <- "test 2 update the title :)"
my_eml$packageId <- "edi.757.2"
EML::write_eml(my_eml, "edi.757.2.xml")

Documentation

list_principal_owner_citations

in the documentation it mentions the principalOwner as being:

(character) Principal owner in the format returned by construct_dn()

I can't seem to find that function. I think this should say create_dn()?

search_data_package

It might be helpful for the documentation to have the formats for some of the fields such as singledate, begindate and pubdate so it can be easily referenced.

create_journal_citation

using the example (and delete_journal_citation example) and got the following error:

journalCitationId <- create_journal_citation(
  packageId = "edi.17.1",
  articleDoi = "https://doi.org/10.1890/11-1026.1",
  articleTitle = "Corridors promote fire via connectivity and edge effects",
  journalTitle = "Ecological Applications",
  relationType = "IsCitedBy",
  env = "staging"
)
# Error in create_journal_citation(packageId = "edi.17.1", articleDoi = "https://doi.org/10.1890/11-1026.1",  : 
#   Bad Request (HTTP 400). Failed to DOI https://doi.org/10.1890/11-1026.1 should be in "prefix/suffix" format (e.g., 10.6073/d4f26a6c).

I think the articleDOI in the example should be "10.1890/11-1026.1".

adamhsparks commented 2 years ago

@laijasmine, thank you for your comprehensive review.

@clnsmth, you can choose to start working on the items that were raised in this review or wait for @bozaah’s review as well before starting.

@bozaah, there is no rush here. The review isn’t due until 2022/02/25, please feel free to take your time.

clnsmth commented 2 years ago

Many thanks for this helpful review @laijasmine!

@adamhsparks, @bozaah, I'll wait for the second review before beginning revisions.

bozaah commented 2 years ago

Package Review

No current or past work/personal relationship to report.

Documentation

The package includes all the following forms of documentation:

Functionality

Estimated hours spent reviewing: ~9h

Review Comments

General Comments

Thank you for the opportunity to review the EDIutils package. This package provides an API client to access the Environmental Data Initiative Repository, in addition to the web browser portal (EDI). It consists of a suite of functions to browse/list, access, evaluate and upload data packages as well as manage citations entries and usage metrics associated with the data packages.

I am reviewing the package using R version 4.1.2 (2021-11-01) and platform x86_64-apple-darwin17.0 (64-bit). No issues reported when running devtools::check(). When running goodpractice::gp() no errors/issues however unit testing warns that 77% of code lines are covered by test cases.

Vignettes

Overall, vignettes are well structured, complete and cover the general use and applicability of the package functions.

Search and Access

No issues with the search and access vignettes. I also mirror comments from @laijasmine regarding XML which, I am not too familiar with and, it is likely the reality of many potential users of the package. There is already a note on Retrieve Download Metrics section which mention that several functions of the EDI API return XML and that xml2 is a reasonable approach to locally manipulate (eg extract and save) EML files.

Several functions do not work until the user has completed the authentication step with login() which requires a EDI account. The vignette Tests Requiring Authentication has information about it but perhaps a vignette on authentication only could be a reasonable addition. If so, I believe it should placed as the first vignette or together with a Get started vignette.

Lastly, mirroring comments of @laijasmine regarding the doi formats, the function read_data_package_doi() returns only the doi identifier eg doi:10.6073/pasta/5d2c5ac4d6cb1293a47cfa6ef5c55a01 but it could perhaps return the complete URL ie https://doi.org/10.6073/pasta/5d2c5ac4d6cb1293a47cfa6ef5c55a01

Evaluate and Upload

No issues here but I also agree with comments about the output of the read_evalute_report() function. However, the function description/example does present @laijasmine's suggestion to use writeLines() and also mention the possibility of alternative output formats with the frmt argument (defaults to xml). I also noted that the read_evalute_report_summary() offers a compact way to visualise the report output, which is quite neat.

Retrieve Download and Citation metrics

These vignettes are well structure and include reasonable examples and potential user cases. Functions and arguments are descriptive and detailed. No errors or issues to report for those vignettes, with exception of the citation doi error in the examples which were already mentioned by @laijasmine.

Documentation and examples

Documentation is automatically generated with roxygen2 and devtools::document(). Functionally is descriptive and complete. R code follows community guidelines (ie tidyverse style guide and rOpenSci package guide). The author also included contribution guidelines separately from the README. Examples run without errors.

Final Remarks

Access to data repositories with accurate and structured/complete metadata is essential for research reproducibility and general data reuse. The ability to obtain usage and citation metrics for the data packages in the EDI repository is outstading. EDIutils offers a great tool for the Open Source community and it will be a welcomed addition to the rOpenSci packages.

adamhsparks commented 2 years ago

@bozaah, thank you for your review.

adamhsparks commented 2 years ago

@ropensci-review-bot submit https://github.com/ropensci/software-review/issues/498#issuecomment-1038707902 time 8

ropensci-review-bot commented 2 years ago

I'm sorry human, I don't understand that. You can see what commands I support by typing:

@ropensci-review-bot help

adamhsparks commented 2 years ago

@ropensci-review-bot submit review https://github.com/ropensci/software-review/issues/498#issuecomment-1038707902 time 8

ropensci-review-bot commented 2 years ago

Logged review for laijasmine (hours: 8)

adamhsparks commented 2 years ago

@ropensci-review-bot submit review https://github.com/ropensci/software-review/issues/498#issuecomment-1048525426 time 9

ropensci-review-bot commented 2 years ago

Logged review for bozaah (hours: 9)

clnsmth commented 2 years ago

Many thanks for these comments @bozaah!

@adamhsparks, I'll begin responding to reviews next week.

clnsmth commented 2 years ago

Thanks again for these helpful reviews @laijasmine @bozaah! I agree with all of your recommendations and will begin implementing them in the development branch of the EDIutils GitHub. The responses and links to corresponding commits will be listed here in this issue thread, and the changes should be completed by the end of Tuesday.

clnsmth commented 2 years ago

Many thanks @laijasmine and @bozaah for your comments and suggested improvments to EDIutils! I've implemented your recommendations and responded to your comments below. Additional comments and suggestions are much appreciated.

Changes to EDIutils since your review:

Response to @laijasmine

You have been added to the EDIutils package DESCRIPTION as a reviewer (see EDIorg/EDIutils@9f3ace8). Please check the accuracy of this information and let me know if anything should be added/changed.

Vignettes

Search and Access

Many users are probably unfamiliar with xml and how to navigate it and there is a learning curve involved so a vignette on the files would be helpful. They might be more comfortable with working with dataframes instead. We don't need to go into detail but it might be helpful to point them to packages like XML to help convert the content to a preferred format. Maybe adding a couple lines or point them to retrieve downloads will be helpful.

I totally agree. The learning curve involved with XML can be off-putting and an unnecessary barrier.

Functions that returned a simple XML structure have a new as parameter through which the return type can be controlled with data.frame as the default (see EDIorg/EDIutils@82f38bc and EDIorg/EDIutils@0441ba2). Unfortunately these simplifications can't be made for more complex returns (e.g. EML) so in these cases a note has been added to the associated function metadata and vignettes pointing users to other resources for more on parsing (see EDIorg/EDIutils@26f888a).

The recommendation for a vignette on working with more complex XML is definitely well recieved, and I would be happy to create it if given a little more time. Until then, I've opened an issue in the project GitHub for this recommendation (see https://github.com/EDIorg/EDIutils/issues/34).

Evaluate and upload

Upload a data package

I struggled the longest here. Maybe because I was just trying to write the least amount of information possible to just upload the file. I had a hard time understanding what the errors were in the results of read_evaluate_report. I went back and forth between the ezEML form and R trying to figure out what I needed. There seems to be more requirements to upload using EDIutils compared to the Check Metadata in ezEML. For example I tried to avoid uploading a data table because it wasn't marked as required in ezEML but it was required to upload using EDIutils.

My apologies for the troubles. This is clearly a short comming in the documentation and possibly the report itself.

A new section on "Interpreting the Evaluation Report" has been added to the Evaluate and Upload Data vignette (see EDIorg/EDIutils@f4a0ab9). This provides more information on the report structure and content. Ultimately the report itself should be updated, it's a bit cryptic in places, but that is a task that will have to be addressed at the repository level.

Report

I also found the output from read_evaluate_report quite long and hard to read if I'm viewing it as a message in the console. We might want to suggest users to use writeLines(report, "report.txt") instead so they can cmd + f the file and keep track of their fixes. Users might prefer if there is a way to only show the warnings and errors so there is less to look through when trying to improve their metadata.

The suggestion for writing the report to file (for ease of viewing) was listed in the original examples of read_evaluate_report() but admittingly burried too deep to be noticed. Ooops. I've reordered the examples with the preferred approaches listed first (see EDIorg/EDIutils@6aaa552).

A suggestion to only return the warnings and errors would make a nice enhancement. I'll implement this if given a little more time. I've opened an issue in the project GitHub so this isn't lost (see https://github.com/EDIorg/EDIutils/issues/35).

Update a data package

Following the example in the vignette I wasn't able to update my package because I keep getting the error:

Error: Attempting to update a data package to revision '1' but an equal or higher revision ('1') already exists in PASTA: edi.757.1

I realized the packageId slot inside the eml file also needs to be updated. We might need to be more explicit in the vignette that the identifier needs to be updated. I used the following EML package here:

my_eml <- EML::read_eml("edi.757.1.xml")
my_eml$dataset$title <- "test 2 update the title :)"
my_eml$packageId <- "edi.757.2"
EML::write_eml(my_eml, "edi.757.2.xml")

Yes, definitely a nuance worth highlighting. I've added a note to the "Update" section of the "Evaluate and Upload" vignette. Additionally, I've elevated the "Update" section level to make visible in the vignette table of contents. See EDIorg/EDIutils@c1647e0.

Documentation

list_principal_owner_citations

in the documentation it mentions the principalOwner as being: (character) Principal owner in the format returned by construct_dn() I can't seem to find that function. I think this should say create_dn()?

Thanks for catching this. Fixed in EDIorg/EDIutils@082b2ab

search_data_package

It might be helpful for the documentation to have the formats for some of the fields such as singledate, begindate and pubdate so it can be easily referenced.

I completely agree and have added formats to the fields requiring one (see EDIorg/EDIutils@ef4b691). Relatedly, I've opened an issue requesting more examples demonstrating common search patterns and Solr syntax (see https://github.com/EDIorg/EDIutils/issues/36).

create_journal_citation

using the example (and delete_journal_citation example) and got the following error:

journalCitationId <- create_journal_citation(
 packageId = "edi.17.1",
 articleDoi = "https://doi.org/10.1890/11-1026.1",
 articleTitle = "Corridors promote fire via connectivity and edge effects",
 journalTitle = "Ecological Applications",
 relationType = "IsCitedBy",
 env = "staging"
)
# Error in create_journal_citation(packageId = "edi.17.1", articleDoi = "https://doi.org/10.1890/11-1026.1",  : 
#   Bad Request (HTTP 400). Failed to DOI https://doi.org/10.1890/11-1026.1 should be in "prefix/suffix" format (e.g., 10.6073/d4f26a6c).

Thanks for finding this. Fixed in EDIorg/EDIutils@743785c

Response to @bozaah

You have been added to the EDIutils package DESCRIPTION as a reviewer (see EDIorg/EDIutils@9f3ace8). Please check the accuracy of this information and let me know if anything should be added/changed.

General Comments

I am reviewing the package using R version 4.1.2 (2021-11-01) and platform x86_64-apple-darwin17.0 (64-bit). No issues reported when running devtools::check(). When running goodpractice::gp() no errors/issues however unit testing warns that 77% of code lines are covered by test cases.

Yes, this catches my eye too. 77% isn't terribly impressive, but it bumps up to 84% when authenticated, and higher when testing data package evalate, upload, and update operations). I'll stay on the look out for ways of improving the test suite.

Vignettes

Search and Access

No issues with the search and access vignettes. I also mirror comments from @laijasmine regarding XML which, I am not too familiar with and, it is likely the reality of many potential users of the package. There is already a note on Retrieve Download Metrics section which mention that several functions of the EDI API return XML and that xml2 is a reasonable approach to locally manipulate (eg extract and save) EML files.

Agreed. XML isn't much fun for infrequent users. Functions now have the option to return a data.frame instead of xml (in most cases). For details see my response to @laijasmine above.

Several functions do not work until the user has completed the authentication step with login() which requires a EDI account. The vignette Tests Requiring Authentication has information about it but perhaps a vignette on authentication only could be a reasonable addition. If so, I believe it should placed as the first vignette or together with a Get started vignette.

Others have requested help on this topic as well. It must need it. : )

A general statement about authentication and which types of functions require it, is now high up in the README (see EDIorg/EDIutils@bd49970). I'd be happy to write up a vignette on authentication if you think more is needed.

Lastly, mirroring comments of @laijasmine regarding the doi formats, the function read_data_package_doi() returns only the doi identifier eg doi:10.6073/pasta/5d2c5ac4d6cb1293a47cfa6ef5c55a01 but it could perhaps return the complete URL ie https://doi.org/10.6073/pasta/5d2c5ac4d6cb1293a47cfa6ef5c55a01

Nice! This function now returns the complete URL through the as_url parameter (see EDIorg/EDIutils@d34018c).

Evaluate and Upload

No issues here but I also agree with comments about the output of the read_evalute_report() function. However, the function description/example does present @laijasmine's suggestion to use writeLines() and also mention the possibility of alternative output formats with the frmt argument (defaults to xml). I also noted that the read_evalute_report_summary() offers a compact way to visualise the report output, which is quite neat.

A new section on "Interpreting the Evaluation Report" has been added to the Evaluate and Upload Data vignette (see EDIorg/EDIutils@f4a0ab9). This provides more information on the report structure and content.

Retrieve Download and Citation metrics

These vignettes are well structure and include reasonable examples and potential user cases. Functions and arguments are descriptive and detailed. No errors or issues to report for those vignettes, with exception of the citation doi error in the examples which were already mentioned by @laijasmine.

Thanks! The fix for the citation doi error is at EDIorg/EDIutils@743785c.

adamhsparks commented 2 years ago

Thank you for the detailed log of the changes that you've made @clnsmth. @laijasmine and @bozaah, when you have time could you please review the response and changes made and let me know if you're satisfied with the changes?

laijasmine commented 2 years ago

Thank you @clnsmth for answering all my questions in detail! I'll be able to probably take a look in the next day or two.

laijasmine commented 2 years ago

The description info is correct for me!

A couple minor revisions before I can approve. Now that some of the functions use the argument as we need to update the vignettes and Readme.

1) For this vignette, the default for get_audit_report has now changed to "data.frame" so we will need to update the code to specify as to be xml. Same goes forlist_data_package_citations in this vignette.

2) In both the readme and the vignette for read_evaluate_report the argument needs to be changed from frmt to as.

I think it is reasonable to leave the bigger changes (https://github.com/EDIorg/EDIutils/issues/34 and https://github.com/EDIorg/EDIutils/issues/35) as issues to be worked on in the future.

All the other changes looks good to me. Thank you for getting all those changes in!

clnsmth commented 2 years ago

Thanks @laijasmine. I'll get these documents updated and link you to the changes once implemented.

adamhsparks commented 2 years ago

@bozaah, did you have any further comments or are you happy with the changes and suggest accepting the package as it's currently written?

bozaah commented 2 years ago

Thanks for following up Adam. I am satisfied with the replies and edits to my points.

Thank you for addressing our proposed changes @clnsmth. Also, I agree with @laijasmine, the bigger changes can be worked out in the future as issues.

clnsmth commented 2 years ago

Thanks @bozaah.

@laijasmine I'll implement your requested changes shortly and post an update when completed.

clnsmth commented 2 years ago

@adamhsparks @laijasmine @bozaah A bug was discovered when rebuilding the two vignettes "Retrieve Download Metrics" and "Retrieve Citation Metrics" (https://github.com/EDIorg/EDIutils/issues/37). I'll send an update once this issue is fixed.

clnsmth commented 2 years ago

@adamhsparks, @laijasmine, @bozaah

laijasmine commented 2 years ago

Looks great! Thank you @clnsmth for all of your work. @adamhsparks I approve this package.

adamhsparks commented 2 years ago

@ropensci-review-bot approve EDIutils

ropensci-review-bot commented 2 years ago

Approved! Thanks @clnsmth for submitting and @bozaah, @laijasmine for your reviews! :grin:

To-dos:

Should you want to acknowledge your reviewers in your package DESCRIPTION, you can do so by making them "rev"-type contributors in the Authors@R field (with their consent).

Welcome aboard! We'd love to host a post about your package - either a short introduction to it with an example for a technical audience or a longer post with some narrative about its development or something you learned, and an example of its use for a broader readership. If you are interested, consult the blog guide, and tag @ropensci/blog-editors in your reply. She will get in touch about timing and can answer any questions.

We maintain an online book with our best practice and tips, this chapter starts the 3d section that's about guidance for after onboarding (with advice on releases, package marketing, GitHub grooming); the guide also feature CRAN gotchas. Please tell us what could be improved.

Last but not least, you can volunteer as a reviewer via filling a short form.

clnsmth commented 2 years ago

Many thanks again to @adamhsparks, @laijasmine, and @bozaah!

I'll get to work on the to-dos.

clnsmth commented 2 years ago

My apologies for the inconvenience @adamhsparks, but can you resend the rOpenSci invitation so I can transfer the repo to the "ropensci" GitHub organization. Thanks!