search

ropensci / software-review

rOpenSci Software Peer Review.
295 stars 104 forks source link

Submission: SymbiotaR2 #373

Closed akoontz11 closed 3 years ago

akoontz11 commented 4 years ago

Submitting Author: Austin Koontz (@akoontz11 )
Repository: https://github.com/pearselab/SymbiotaR2 Version submitted: 0.0.1 Editor: !--editor-->@noamross<!--end-editor-- Reviewers: @bpbond, @sckott

Due date for @bpbond: 2020-06-25 Due date for @sckott: 2020-08-31

Archive: TBD
Version accepted: TBD 

Package: SymbiotaR2
Type: Package
Title: Downloading data from Symbiota2 portals into R
Version: 0.0-1
Authors@R: c(person("Austin", "Koontz", email = "austin.koontz@usu.edu", role = c("aut", "cre")),
    person("William D.", "Pearse", email = "will.pearse@gmail.com", role ="aut", comment=c(ORCID="0000-0002-6241-3164")))
Maintainer: Austin Koontz <austin.koontz@usu.edu>
Description: Download data from Symbiota2 portals using Symbiota's
    API. Covers the Checklists, Collections, Crowdsource, Exsiccati,
    Glossary, ImageProcessor, Key, Media, Occurrence, Reference, Taxa,
    Traits, and UserRoles API families. Each Symbiota2 portal owner
    can load their own plugins (and modified code), and so this
    package may not cover every possible API endpoint from a given
    Symbiota2 instance.
License: MIT + file LICENSE
URL: https://github.com/pearselab/SymbiotaR2
BugReports: https://github.com/pearselab/SymbiotaR2/issues
VignetteBuilder: knitr
LazyLoad: yes
Suggests:
    knitr (>= 1.23),
    testthat (>= 2.0.0),
    vcr (>= 0.3.0)  
Imports:
    httr (>= 1.0.0),
    xml2 (>= 1.2.0),
    rjson (>= 0.2.20),
    fs (>= 1.2.6)
Encoding: UTF-8
RoxygenNote: 7.0.2

Scope

The primary purpose of SymbiotaR2 is to allow users to pull objects from the specified Symbiota2 database portal into an R environment. It does this by wrapping each of the default API calls made available on the Symbiota2 portal of interest.

The target audiences includes anyone interested in using R to analyze biological specimen data made available through the Symbiota2 content management system. Potential scientific applications include comparing biological trait data between different lineages, analyzing phenological data, and biodiversity research generally.

No R packages have been developed for the updated code structure provided by Symbiota2.

Technical checks

Confirm each of the following by checking the box.

This package:

Publication options

JOSS Options - [x] The package has an **obvious research application** according to [JOSS's definition](https://joss.readthedocs.io/en/latest/submitting.html#submission-requirements). - [x] The package contains a `paper.md` matching [JOSS's requirements](https://joss.readthedocs.io/en/latest/submitting.html#what-should-my-paper-contain) with a high-level description in the package root or in `inst/`. - [ ] The package is deposited in a long-term repository with the DOI: - (*Do not submit your package separately to JOSS*)
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

akoontz11 commented 4 years ago

Tagging @willpearse as a contributing author of this package as well.

noamross commented 4 years ago

Editor checks:

Editor comments

Thank you for your submission @akoontz11!

Here is output from goodpractice::gp(). Your test coverage is good, and the >80 lines are mostly just due to indention conventions. I note that you use sapply() in basically the same way all over the package and it might make sense to make a small utility function replacing this use that uses vapply(), which might also make code clearer. I also note that the documentation can be a little terse; README or other documentation might go into more detail, as to the the nature of Symbiota data and data types following the principle of mutiple points of entry. None of these things require holding up review; I am seeking reviewers and will ask them to evaluate these observations.

── GP SymbiotaR2 ────────────────────────────

It is good practice to

  ✖ write unit tests for all functions, and all package code
    in general. 87% of code lines are covered by test cases.

    R/Defaults.R:28:NA
    R/Defaults.R:31:NA
    R/Defaults.R:32:NA
    R/Defaults.R:33:NA
    R/Defaults.R:34:NA
    ... and 57 more lines

  ✖ avoid long code lines, it is bad for readability. Also,
    many people prefer editor windows that are about 80 characters
    wide. Try make your lines shorter than 80 characters

    R/Checklists.R:3:1
    R/Checklists.R:10:1
    R/Checklists.R:11:1
    R/Checklists.R:21:1
    R/Checklists.R:43:1
    ... and 99 more lines

  ✖ avoid sapply(), it is not type safe. It might return a
    vector, or a list, depending on the input data. Consider using
    vapply() instead.

    R/Checklists.R:24:13
    R/Checklists.R:32:20
    R/Checklists.R:70:17
    R/Checklists.R:90:13
    R/Checklists.R:98:20
    ... and 14 more lines

Reviewers: @abarner @bpbond Due date: 2020-06-25

willpearse commented 4 years ago

Brilliant, thank you Dr Ross! If it's OK with you, can we wait until after the reviews are back in to address your sapply and documentation suggestions? I imagine there will be other comments and it might be easier to address everything at once.

noamross commented 4 years ago

⚠️⚠️⚠️⚠️⚠️

In the interest of reducing load on reviewers and editors as we manage the COVID-19 crisis, rOpenSci is temporarily pausing new submissions for software peer review for 30 days (and possibly longer). Please check back here again after 17 April for updates.

In this period new submissions will not be handled, nor new reviewers assigned. Reviews and responses to reviews will be handled on a 'best effort' basis, but no follow-up reminders will be sent.

Other rOpenSci community activities continue. We express our continued great appreciation for the work of our authors and reviewers. Stay healthy and take care of one other.

The rOpenSci Editorial Board

⚠️⚠️⚠️⚠️⚠️

annakrystalli commented 4 years ago

⚠️⚠️⚠️⚠️⚠️ In the interest of reducing load on reviewers and editors as we manage the
COVID-19 crisis, rOpenSci new submissions for software peer review are paused.

In this period new submissions will not be handled, nor new reviewers assigned.
Reviews and responses to reviews will be handled on a 'best effort' basis, but
no follow-up reminders will be sent. Other rOpenSci community activities continue.

Please check back here again after 25 May when we will be announcing plans to slowly start back up.

We express our continued great
appreciation for the work of our authors and reviewers. Stay healthy and take
care of one other.

The rOpenSci Editorial Board ⚠️⚠️⚠️⚠️⚠️

noamross commented 4 years ago

Hello @akoontz11 and @willpearse, we're starting up review activities again so I'll be seeking reviewers now, but I wanted to check in and see if there are any developments. Anything I should know about updates to the package or your schedule? Thanks for your patience on this!

willpearse commented 4 years ago

Thanks, but there have been no changes. No worries about the delay.

noamross commented 4 years ago

My apologies this is taking a while. I've been digging through the various Symbiota portal organizations looking for people who use it who may be reviewers. Do you know any Symbiota users/administrators to suggest as reviewers?

willpearse commented 4 years ago

Thanks Noam. I do know some, but I worry that suggesting one of my friends would be a bit unethical! Have you found this list ( http://symbiota.org/docs/symbiota-introduction/active-symbiota-projects/)? I should add that many herbarium managers are not R developers, and so they might be unwilling to review an R package (they're often taxonomic experts). Another option might be someone who has spent a lot of time working with natural history collection data; an example would be Emily Meinke (although we shared a post-doc advisor; https://emilykmeineke.com/).

noamross commented 4 years ago

Thanks Will. I've found two reviewers. Thank you @abarner and @bpbond for agreeing to review! Due date is 2020-06-25

willpearse commented 4 years ago

Brilliant! Sorry that I wasn't very helpful there; I'm moving country right now so I'm a bit all over the place at the moment to be honest with you.

On Thu, 4 Jun 2020 at 17:50, Noam Ross notifications@github.com wrote:

Thanks Will. I've found two reviewers. Thank you @abarner https://github.com/abarner and @bpbond https://github.com/bpbond for agreeing to review! Due date is 2020-06-25

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ropensci/software-review/issues/373#issuecomment-638977301, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJNYURDPRHXPAISFR7UBFTRU7GGXANCNFSM4LAR6H6A .

bpbond commented 4 years ago

@willpearse @akoontz11 What's the workflow for running the package tests? I've tried devtools::test() and source'ing tests/test-all.R, and get errors both ways. Thanks.

...as well as testthat::test_package

akoontz11 commented 4 years ago

Hi @bpbond, thank you for agreeing to review this package!

Here's the workflow we've used for installation and testing:

Rscript -e "require(roxygen2);roxygenize('~/Symbiota2/code/SymbiotaR2/')"
R CMD build ~/Symbiota2/code/SymbiotaR2
Rscript -e "install.packages('SymbiotaR2_0.0.1-tar.gz')"
R CMD check --as-cran SymbiotaR2_0.0-1.tar.gz

Could you let me know what errors you're getting, using either devtools::test("SymbiotaR2"), test_package("SymbiotaR2"), or source'ing test-all.R? I had to call test_check("SymbiotaR2") from within the tests directory, in order to get that method to work. We're using Travis-CI and covr to report test coverage, which is ~87%.

bpbond commented 4 years ago

Using R CMD check as above does run the tests successfully. I can't get test_check() to work regardless of the working directory. Anyway, thanks, and more in a bit.

bpbond commented 4 years ago

Apologies for the delay.

Package Review

Documentation

The package includes all the following forms of documentation:

For packages co-submitting to JOSS

The package contains a paper.md matching JOSS's requirements with:

  • [X] A short summary describing the high-level functionality of the software
  • [X] Authors: A list of authors with their affiliations
  • [X] A statement of need clearly stating problems the software is designed to solve and its target audience.
  • [ ] References: with DOIs for all those that have one (e.g. papers, datasets, software).

Functionality

Final approval (post-review)

Estimated hours spent reviewing: 2

Review Comments

Biodiversity researchers and institutions face formidable challenges in integrating biological community knowledge and data, and this package aims to provide an easy-to-use interface to Symbiota2. Thus I appreciate (after some research) the goals of this package, and the work the authors have put into it.

That said, it seems like there's a fair amount of work that could be done on this package to make it more accessible for beginners and non-specialists (or specialists unfamiliar with R packages), increase its robustness, and generally conform better to best practices. Many of the limitations below made it impossible for me, as a reviewer, to fully test or understand the package's functionality.

The comments below, which I hope are useful for the authors, are grouped into broad categories for readability:

Building

R CMD check` gives the following note:

The Title field should be in title case. Current version is:
‘Downloading data from Symbiota2 portals into R’

Package dependencies are minimal and sensible.

I highly recommend that @akoontz11 get an ORCID ID and add it to the DESCRIPTION file.

Documentation

The package's documentation root could be made much more useful and welcoming. Here it is: Screen Shot 2020-07-09 at 5 39 05 AM

By way of comparison, here is the documentation home for the zoo package: Screen Shot 2020-07-09 at 5 38 53 AM

The latter provides easy-to-find links to documentation, demos, vignettes, and NEWS. I'd recommend adopting this as a model for Symbiota2.

WHAT IS SYMBIOTA2? The README and main package help pages have nothing about the overall context behind, and need for, this package. The vignette gives slightly more background info but still not enough. I suggest that the authors include something like this (adapted from the Symbiota2 website):

Symbiota is open source software that enables US natural history collections to share information from their collections in ways that benefit multiple user groups.

The package documentation should reference both the Symbiota2 website and the Gries et al. 2014 paper linked therein.

The vignette is quite bare-bones. In addition, its purported output is statically pasted into the Rmd source file, i.e. it's not really being run. OK, but when I tried to run the very first code example in the vignette, got an error than the URL couldn't be reached. As a beginning user, now what do I do?

Most function do not have provided examples.

Code quality

The package code is generally clean, clearly and consistently formatted, and easy to follow.

There's no defensive programming that I can see, e.g. assertions on function entry or exit. This might be a useful addition.

I appreciate that testing network-oriented functions is a challenge. It would be useful if the package test code itself had bit of documentation, in particular information about the testing strategy and what's going on with the YAML files in the tests/testthat folder.

akoontz11 commented 4 years ago

Hi @bpbond, thanks very much for taking the time to put together this review and for your feedback! I agree with everything mentioned here: the package documentation could be more informative, and I think that providing documentation for the testing code is an excellent idea, given the importance of those tests for users trying to connect to a portal. I think these changes will greatly improve the package.

If possible, it'd be helpful for me to receive feedback from the other reviewer before incorporating these changes, so that I could make all suggested changes at once. Thank you again, for this.

noamross commented 4 years ago

@akoontz11 I've contacted the other reviewer again and haven't heard back. If I don't hear back soon I or someone from the team will do a rapid review.

noamross commented 4 years ago

As we've not heard back from our other reviewer, @sckott is doing a rapid review.

willpearse commented 4 years ago

Great, thanks very much.

sckott commented 4 years ago

Package Review

Documentation

The package includes all the following forms of documentation:

For packages co-submitting to JOSS

The package contains a paper.md matching JOSS's requirements with:

  • [x] A short summary describing the high-level functionality of the software
  • [x] Authors: A list of authors with their affiliations
  • [x] A statement of need clearly stating problems the software is designed to solve and its target audience.
  • [ ] References: with DOIs for all those that have one (e.g. papers, datasets, software). . - the 1 reference that probably has a DOI does not include a DOI

Functionality

Final approval (post-review)

Estimated hours spent reviewing: 1.5

Review Comments

akoontz11 commented 4 years ago

Hi @sckott and @bpbond, thanks very much for all of the helpful feedback on SymbiotaR2! I'm working on each of these points now and will post in this thread once these changes have been incorporated.

akoontz11 commented 4 years ago

@bpbond and @sckott: firstly, thank you again for reviewing SymbiotaR2 and providing all of the helpful feedback! I've copied each of your review comments and pasted them below, along with responses on points where you've recommended changes (headed with "AK:"). For some of these points, I've included explanations as to why we haven't addressed the issue.

Please let me know if there's anything I can do to clarify any of my points below, and again, thanks for contributing your help.

@bpbond

Apologies for the delay.

Package Review

  • Briefly describe any working relationship you have (had) with the package authors.
  • [x] As the reviewer I confirm that there are no conflicts of interest for me to review this work

Documentation

The package includes all the following forms of documentation:

  • [ ] A statement of need clearly stating problems the software is designed to solve and its target audience in README AK: The README has been updated to provide a statement of need for SymbiotaR2.
  • [x] Installation instructions: for the development version of package and any non-standard dependencies in README
  • [x] Vignette(s) demonstrating major functionality that runs successfully locally
  • [x] Function Documentation: for all exported functions in R help
  • [ ] Examples for all exported functions in R Help that run successfully locally
  • [ ] Community guidelines including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL, BugReports and Maintainer (which may be autogenerated via Authors@R). AK: A CONTRIBUTING.md file has been provided within a .github directory
For packages co-submitting to JOSS

The package contains a paper.md matching JOSS's requirements with:

  • [x] A short summary describing the high-level functionality of the software
  • [x] Authors: A list of authors with their affiliations
  • [x] A statement of need clearly stating problems the software is designed to solve and its target audience.
  • [ ] References: with DOIs for all those that have one (e.g. papers, datasets, software).

Functionality

  • [x] Installation: Installation succeeds as documented.
  • [ ] Functionality: Any functional claims of the software been confirmed.
  • [ ] Performance: Any performance claims of the software been confirmed.
  • [x] Automated tests: Unit tests cover essential functions of the package and a reasonable range of inputs and conditions. All tests pass on the local machine.
  • [x] Packaging guidelines: The package conforms to the rOpenSci packaging guidelines

Final approval (post-review)

  • [ ] The author has responded to my review and made changes to my satisfaction. I recommend approving this package.

Estimated hours spent reviewing: 2

  • [ ] Should the author(s) deem it appropriate, I agree to be acknowledged as a package reviewer ("rev" role) in the package DESCRIPTION file.

Review Comments

Biodiversity researchers and institutions face formidable challenges in integrating biological community knowledge and data, and this package aims to provide an easy-to-use interface to Symbiota2. Thus I appreciate (after some research) the goals of this package, and the work the authors have put into it.

That said, it seems like there's a fair amount of work that could be done on this package to make it more accessible for beginners and non-specialists (or specialists unfamiliar with R packages), increase its robustness, and generally conform better to best practices. Many of the limitations below made it impossible for me, as a reviewer, to fully test or understand the package's functionality.

The comments below, which I hope are useful for the authors, are grouped into broad categories for readability:

Building

R CMD check gives the following note:

The Title field should be in title case. Current version is:
‘Downloading data from Symbiota2 portals into R’

AK: Thanks for this, @bpbond --the Title field had been updated to use title case

Package dependencies are minimal and sensible.

I highly recommend that @akoontz11 get an ORCID ID and add it to the DESCRIPTION file.

AK: @akoontz11's ORCID ID has been added to the DESCRIPTION file.

Documentation

The package's documentation root could be made much more useful and welcoming. Here it is: Screen Shot 2020-07-09 at 5 39 05 AM

By way of comparison, here is the documentation home for the zoo package: Screen Shot 2020-07-09 at 5 38 53 AM

The latter provides easy-to-find links to documentation, demos, vignettes, and NEWS. I'd recommend adopting this as a model for Symbiota2.

AK: Thanks for this recommendation. The documentation head has been updated to include a link to the vignette, and a NEWS.md file has been included in the root directory.

WHAT IS SYMBIOTA2? The README and main package help pages have nothing about the overall context behind, and need for, this package. The vignette gives slightly more background info but still not enough. I suggest that the authors include something like this (adapted from the Symbiota2 website):

Symbiota is open source software that enables US natural history collections to share information from their collections in ways that benefit multiple user groups.

The package documentation should reference both the Symbiota2 website and the Gries et al. 2014 paper linked therein.

The vignette is quite bare-bones. In addition, its purported output is statically pasted into the Rmd source file, i.e. it's not really being run. OK, but when I tried to run the very first code example in the vignette, got an error than the URL couldn't be reached. As a beginning user, now what do I do?

AK: Thanks for this, @bpbond. The README has been updated to provide more context for SymbiotaR2, and an R/SymbiotaR2-package.R file has also been included to provide more information within the package documentation. The vignette has also been updated to include more sections (similar to the README). Both include links to the Symbiota website and the Gries et al. 2014 paper. Finally, the vignette includes some description of the package code, as well as a Troubleshooting section. Because Symbiota2 is still in development, there currently isn't a public-facing portal we could direct users to, so we can't provide a URL that users can work with right now. We appreciate and agree that this is far from ideal, but because we're trying to get everything 'feature complete' for the launch of Symbiota2, we're working towards having the R package ready, citable, and online for when existing portals upgrade to the new version of Symbiota.

Most function do not have provided examples.

AK: This has been updated, and examples are now included in the help files, with one for each API family of functions.

Code quality

The package code is generally clean, clearly and consistently formatted, and easy to follow.

There's no defensive programming that I can see, e.g. assertions on function entry or exit. This might be a useful addition.

AK: Thanks for recommending this. Parameter type checking has been incorporated for several of the package's root functions (i.e. .check.url and .api.scaffold--but please note that id arguments do not use parameter type checking, as some Symbiota2 API handles utilize a non-numeric id)

I appreciate that testing network-oriented functions is a challenge. It would be useful if the package test code itself had bit of documentation, in particular information about the testing strategy and what's going on with the YAML files in the tests/testthat folder.

AK: I totally agree, I think there's a clear value in being more explicit regarding the testing. To address this, Testing sections have been included in both the README and the vignette. There are some additional complexities to this beyond network-oriented testing (see above the comment about the chicken-and-egg problem of portals not yet having upgraded to Symbiota2).

@sckott

Package Review

  • Briefly describe any working relationship you have (had) with the package authors.
  • [x] As the reviewer I confirm that there are no conflicts of interest for me to review this work (If you are unsure whether you are in conflict, please speak to your editor before starting your review).

Documentation

The package includes all the following forms of documentation:

  • [ ] A statement of need clearly stating problems the software is designed to solve and its target audience in README AK: The README has been updated to provide a statement of need for SymbiotaR2.
  • [x] Installation instructions: for the development version of package and any non-standard dependencies in README
  • [x] Vignette(s) demonstrating major functionality that runs successfully locally
  • [x] Function Documentation: for all exported functions in R help

  • [ ] Examples for all exported functions in R Help that run successfully locally

    • there's no examples other than for the setup fxn. I'd normally say there should be egs for every exported function, but if there's no public symbiota instance to access then that makes including examples hard.

AK: Examples have now been included for each API family of SymbiotaR2 functions (see example here). That being said, we definitely agree that a public Symbiota2 instance would be really useful for explaining function outputs--but unfortunately, there's no instance we could reference for that, right now (see above).

  • [ ] Community guidelines including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL, BugReports and Maintainer (which may be autogenerated via Authors@R).

    • ideally make a .github dir and include in there contributing.md, as well as issue and pull request templates

AK: Thanks for this suggestion, this is very helpful. A .github directory has been added to the root folder, and includes a CONTRIBUTING.md file

For packages co-submitting to JOSS

The package contains a paper.md matching JOSS's requirements with:

  • [x] A short summary describing the high-level functionality of the software
  • [x] Authors: A list of authors with their affiliations
  • [x] A statement of need clearly stating problems the software is designed to solve and its target audience.
  • [ ] References: with DOIs for all those that have one (e.g. papers, datasets, software). . - the 1 reference that probably has a DOI does not include a DOI

AK: Good catch: a DOI has been included for the Gries et al. 2014 paper.

Functionality

  • [x] Installation: Installation succeeds as documented.
  • [ ] Functionality: Any functional claims of the software been confirmed.
  • [ ] Performance: Any performance claims of the software been confirmed.

  • [ ] Automated tests: Unit tests cover essential functions of the package and a reasonable range of inputs and conditions. All tests pass on the local machine.

    • tests do not run for me. some suggestions:

    • from an organization perspective, its a good idea to put your vcr cassettes into their own folder, and tests in their own folder. if you run vcr::use_vcr() it will set things up in the suggested way with the cassettes in tests/fixtures and test files in test/testhat. https://github.com/ropensci/vcr#getting-started

AK: Thanks for detailing this, @sckott. The tests directory has been updated to follow this format, and a helper-SymbiotaR2.R file has been included in the tests/testthat directory.

* probably best to namespace the `use_cassette` calls, so use `vcr::use_cassette` - running tests errors for me as R doesn't know where to find `use_cassette`

AK: Great point. All use_cassette commands are now namespaced

* to get the most accurate information on what lines errors occur on in tests, better to wrap just the functions that make http requests in use_cassette and then the testthat expectations below that - optional of course

AK: Thanks for this recommendation, as this should make future troubleshooting easier. Tests have been updated to only wrap SymbiotaR2 commands in use_cassette (example here)

  • [ ] Packaging guidelines: The package conforms to the rOpenSci packaging guidelines

    • see comments below

Final approval (post-review)

  • [ ] The author has responded to my review and made changes to my satisfaction. I recommend approving this package.

Estimated hours spent reviewing: 1.5

  • [ ] Should the author(s) deem it appropriate, I agree to be acknowledged as a package reviewer ("rev" role) in the package DESCRIPTION file.

Review Comments

  • agree with 1st reviewer that the pkg would greatly benefit from parameter type checking - e.g., assert(some_parameter, "character")

AK: See response above: parameter type checking incorporated for .check.url and .api.scaffold functions (but id parameters are not checked to be numeric, as a few functions use a non-numeric id argument).

  • the functions throughout the pkg use NA as default. this isn't covered per se in ropensci guidelines, but this strikes me as out of the ordinary. often fxns have default as missing or NULL, with functions to check those things within the function, e.g., missing(param) or is.null(param).

AK: This has been updated: missing arguments now utilize NULL as the default, rather than NA (example here)

  • there's a mix of styles with parameters and functions. for parameters, some are all lowercase, some all uppercase. making all lowercase would make sense, and follow the guidelines. functions are consistent though, all being TitleCase

AK: Thanks for calling this out. All function parameters are now in lowercase. Function names remain unchanged.

  • maintainers could make their work easier using @inheritParams or @template to reduce redundant parameter definitions

AK: Thank you so much for this suggestion! @template is now being used to build help pages for most package functions.

  • is it true that a user using this pkg would likely only use one url per R session? if so, maybe it would make sense to put the url parameter as the last one in each function as the user is not likely to set the value of that parameter, so they could use more succinct code by only setting the id and/or page

AK: Great point--url has been moved to be the last argument specified for all functions.

  • with SymbiotaR2_setup() i can set any arbitrary URL that exists. can the url check be changed to make sure the url is for a symbiota instance?

AK: Good point. To address this, we have incorporated an API call at the end of the .check.url function (call for Occurrences(id=1)).

  • are there any public symbiota instances? if so that'd be useful to make it possible for users to try out the package more easily

AK: Thanks for this recommentation, @sckott. We agree that public instance of Symbiota2 would greatly help to simplify the SymbiotaR2 package documentation (and would be generally helpful for Symbiota2 usage). While this is something we've mentioned to the development team, there currently isn't a public Symbiota2 instance available. As we outline above, we are trying to build the infrastructure that supports Symbitoa2 portals at the same time as the software itself, so that when we have an official "launch day" managers can upgrade their portals and everything will "just work". We appreciate this makes things much more difficult for the reviewers and we do apologize about that!

  • strongly recommend building the readme.md from a readme.rmd and building the vignette from actual code - you'd probably want to prebuild the vignette for this pkg though, see e.g., this post

AK: We absolutely agree that this would be ideal, but because no public Symbiota2 instances currently exist, we have decided not to do this since it will make pull requests and group development around the package more difficult. Because this is a very good idea, we have added an issue to the repository with an 'enhancement' label. Once Symbiota2 publicly launches (or at least the public demo site), we will make this improvement.

  • code style: lots of lines in the package > 80 width, not super important, but improves readability if lines are 80 char or less

AK: To address this, I've used lintr to clean up the code in this package.

AK: Thanks for this--we now mention rSymbiota in the README and the vignette, and mention the existence of other packages as well. Because SymbiotaR2 is the only R package developed for interfacing with the refactored Symbiota2 code, there's no overlap between existing packages (for Symbiota) and SymbiotaR2.

  • no top level manual file exists - per guidelines: "The package should contain top-level documentation for ?foobar"

AK: Thanks for this recommendation--a R/SymbiotaR2-package.R file has now been included

bpbond commented 4 years ago

@noamross Do I provide assessment of authors' responses to my comments?

noamross commented 4 years ago

My apologies for missing this last exchange and letting this linger. Thank you @sckott and @bpbond for your thourough reviews and @akoontz11 for your response. @bpbond and @sckott, can you please look at @akoontz11's replies and us know if they address the concerns in your review.

bpbond commented 4 years ago

I haven't actually installed and tested the revised package (should I?), but @akoontz11 's responses above address all my concerns. Very comprehensive and convincing.

sckott commented 4 years ago

Thanks @akoontz11 - all looks good to me

noamross commented 4 years ago

Thank you for your responses, @bpbond and @sckott!

Approved! Thanks @akoontz11 for submitting and @bpbond and @sckott for your reviews!

To-dos:

For JOSS submission:

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). More info on this here.

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 @stefaniebutland in your reply. She will get in touch about timing and can answer any questions.

We've put together an online book with our best practice and tips, this chapter starts the 3d section that's about guidance for after onboarding. Please tell us what could be improved, the corresponding repo is here.

akoontz11 commented 4 years ago

Hi @noamross, thanks for this checklist, the guidance, and the approval! I believe I've made most of the necessary updates you've listed, but let me know if anything looks astray.

@bpbond and @sckott, I would like to include you as rev authors for the package--if that is alright with you, let me know and I will updated the DESCRIPTION file. Once I do, I can tag and create a release for Zenodo.

sckott commented 4 years ago

@akoontz11 thx, would be honored

bpbond commented 4 years ago

Me too--not at all necessary but appreciated!