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

submission: rppo #207

Closed
11 of 19 tasks
jdeck88 opened this issue Mar 31, 2018 · 25 comments
Closed
11 of 19 tasks

submission: rppo #207

jdeck88 opened this issue Mar 31, 2018 · 25 comments

Comments

@jdeck88
Copy link

jdeck88 commented Mar 31, 2018

Summary

  • What does this package do? (explain in 50 words or less):
    The global plant phenology data portal (PPO data portal) is an aggregation of plant phenological observations from USA-NPN, NEON, and PEP725 representing 20 million phenological observations from across North America and Europe. The PPO data portal utilizes the Plant Phenology Ontology to align phenological terms and measurements from the various databases. The rppo R package enables programmatic access to all data contained in the PPO data portal.

  • Paste the full DESCRIPTION file inside a code block below:

Package: rppo
Title: R functions to access Plant Phenology Ontology annotated datasets
Date: 2018-03-21
Version: 1.0
Authors@R: as.person(c(
    "John Deck <[email protected]> [aut, cre]", 
    "Brian Stucky <[email protected]> [aut]",
    "Ramona Walls <[email protected]> [aut]",
    "Kjell Bolmgren <[email protected]> [aut]",
    "Ellen Denny <[email protected]> [aut]",
    "Robert Guralnick <[email protected]> [aut]"
  ))
Maintainer: John Deck <[email protected]>
Description: Access data from the Plant Phenology Ontology data store.
Depends: R (>= 3.4.2)
License: CC0
Encoding: UTF-8
LazyData: true
RoxygenNote: 6.0.1.9000
Imports:
    rjson,
    readr,
    plyr,
    leafletR (>= 0.1-1),
    httr
Suggests: testthat,
    knitr,
    rmarkdown
VignetteBuilder: knitr
  • URL for the package (the development repository, not a stylized html page):

https://github.com/biocodellc/rppo/

  • Please indicate which category or categories from our package fit policies this package falls under *and why(? (e.g., data retrieval, reproducibility. If you are unsure, we suggest you make a pre-submission inquiry.):

This package fits under data retrieval since it is used in downloading/extracting data from the global plant phenology data portal.

  •   Who is the target audience and what are scientific applications of this package?  

The target audience are scientists researching plant phenology patterns, shifts of plant phenological patterns over time and space, climate change biologists, and biodiversity scientists looking for sources of plant observation data.

There are no other R packages that we know of that do the same thing.

  •   If you made a pre-submission enquiry, please paste the link to the corresponding issue, forum post, or other discussion, or @tag the editor you contacted.

We enquired with @sckott and @karthik several weeks ago.

Requirements

Confirm each of the following by checking the box. This package:

  • does not violate the Terms of Service of any service it interacts with.
  • has a CRAN and OSI accepted license.
  • contains a README with instructions for installing the development version.
  • includes documentation with examples for all functions.
  • contains a vignette with examples of its essential functions and uses
  • has a test suite.
  • has continuous integration, including reporting of test coverage, using services such as Travis CI, Coveralls and/or CodeCov.
  • I agree to abide by ROpenSci's Code of Conduct during the review process and in maintaining my package should it be accepted.

Publication options

  • Do you intend for this package to go on CRAN?
  • Do you wish to automatically submit to the Journal of Open Source Software? If so:
    • The package has an obvious research application according to JOSS's definition.
    • The package contains a paper.md matching JOSS's requirements 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)
  • Do you wish to submit an Applications Article about your package to Methods in Ecology and Evolution? If so:
    • 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)
    • (Scope: Do consider MEE's Aims and Scope for your manuscript. We make no gaurantee that your manuscript willl 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)

Detail

  • Does R CMD check (or devtools::check()) succeed? Paste and describe any errors or warnings:

  • Does the package conform to rOpenSci packaging guidelines? Please describe any exceptions:

  • If this is a resubmission following rejection, please explain the change in circumstances:

  • If possible, please provide recommendations of reviewers - those with experience with similar packages and/or likely users of your package - and their GitHub user names:

@annakrystalli
Copy link
Contributor

annakrystalli commented Apr 9, 2018

Editor checks:

  • Fit: The package meets criteria for fit and overlap
  • Automated tests: Package has a testing suite and is tested via Travis-CI or another CI service.
  • License: The package has a CRAN or OSI accepted license
  • Repository: The repository link resolves correctly
  • Archive (JOSS only, may be post-review): The repository DOI resolves correctly
  • Version (JOSS only, may be post-review): Does the release version given match the GitHub release (v1.0.0)?

Editor comments

Hello @jdeck88, sorry for the slight delay and many thanks for your submission! An interesting package.

So there's a few issues flagged by the initial editol checks:

testthat.R missing

The package repository is missing a testthat.R script in the tests/ directory. Because of this, running covr::package_coverage() returns 0% coverage. Using usethis::use_testthat() creates the required file and the tests run as expected.

  • Create testthat.R file.
  • Add test coverage as part of CI and a badge for it in the README. (See ?usethis::use_coverage())

goodpractice::gp output

── GP rppo ──────────────────────────────────────────────────────────────────────────────────────
  It is good practice to
 
  ✖ write unit tests for all functions, and all package code in general. 75%
    of code lines are covered by test cases.

    R/ppo_data.R:74:NA
    R/ppo_data.R:75:NA
    R/ppo_data.R:76:NA
    R/ppo_data.R:77:NA
    R/ppo_data.R:78:NA
    ... and 23 more lines
  • reviewers note highlighted coverage gaps that, inspect what is not covered to see if you think important areas are missed.
  ✖ omit "Date" in DESCRIPTION. It is not required and it gets invalid quite
    often. A build date will be added to the package when you perform `R CMD build` on
    it.
     
  • omit "Date" in DESCRIPTION
  ✖ add a "URL" field to DESCRIPTION. It helps users find information about
    your package online. If your package does not have a homepage, add an URL to
    GitHub, or the CRAN package package page.
  • add a "URL" field to DESCRIPTION
  ✖ add a "BugReports" field to DESCRIPTION, and point it to a bug tracker.
    Many online code hosting services provide bug trackers for free,
    https://github.com, https://gitlab.com, etc.
  • add a "BugReports" field to DESCRIPTION
  ✖ use '<-' for assignment instead of '='. '<-' is the standard, and R
    users and developers are used it and it is easier to read your code for them if
    you use '<-'.

    R/ppo_data.R:37:19
    R/ppo_data.R:39:18
    R/ppo_data.R:54:11
    R/ppo_data.R:74:12
    R/ppo_data.R:75:12
    ... and 11 more lines

  • Change '=' assignement to '<-'
  ✖ 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/ppo_data.R:33:1
    R/ppo_data.R:39:1
    R/ppo_data.R:50:1
    R/ppo_data.R:58:1
    R/ppo_data.R:72:1
    ... and 5 more lines

  • check code line length
  ✖ omit trailing semicolons from code lines. They are not needed and most R
    coding standards forbid them

    R/ppo_data.R:41:64
    R/ppo_data.R:119:32
    R/ppo_data.R:120:17
    R/ppo_terms.R:35:59

  • omit trailing semicolons
  ✖ not import packages as a whole, as this can cause name clashes between
    the imported packages. Instead, import only the specific functions you need.
  • reviewers to consider and give feedback on
  ✖ fix this R CMD check NOTE: Namespace in Imports field not imported from:
    ‘leafletR’ All declared Imports should be used.
  • Needs checking. Usually this means you have a package (leafletR) in Imports that should really be in Suggests (perhaps it is only used in a vignette).
    
    
───────────────────────────────────────────────────────────────────────────────────────────────── 
> 

devtools::spell_check()

Flags some potential spelling errors of which only this looks genuine:

pouplates      ppo_terms.Rd:20

CC0 License.

Although the CC0 license specified in the DESCRIPTION is accepted by CRAN, it is not accepted by OSI. We recommend avoiding it unless you have a specific need to use it and suggest considering MIT as a similarly permissible license.


Happy to start looking for reviewers. I recommend using this time to try and address some of the issues raised so they can be checked during review rather that be reflagged.

Let me know if anything is unclear!


Reviewers:
Due date:

@jdeck88
Copy link
Author

jdeck88 commented Apr 10, 2018

Thank you for the comments Anna..
I have addressed all of the issues that you brought up and committed my changes to our Github repo.

@annakrystalli
Copy link
Contributor

Thanks for your prompt response @jdeck88! I've found the first reviewer and just trying to secure the second one. Will keep you posted. 👍

@annakrystalli
Copy link
Contributor

Both reviewers have now been assigned! 🎉


Reviewers: @tdjames1 & @remsamp
Due date: 2018-05-04

@annakrystalli
Copy link
Contributor

annakrystalli commented Apr 30, 2018

👋 @tdjames1 & @remsamp. Just checking in to make sure you're review is going ok? Feel free to reach out if have any questions 😊

@tdjames1
Copy link

tdjames1 commented May 3, 2018

@annakrystalli I'm just getting onto the review now, apologies for the delay. I'm aiming to get it done over this weekend.

@annakrystalli
Copy link
Contributor

annakrystalli commented May 4, 2018

No problem @tdjames1, thanks for letting me know. I'll push back due date to the 7th then.


Reviewers: @tdjames1 & @remsamp
Updated due date: 2018-05-07

@remsamp
Copy link

remsamp commented May 7, 2018

Hi @annakrystalli and @jdeck88, here's my review (apologies for the slight delay).

Package Review

  • 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
  • Installation instructions: for the development version of package and any non-standard dependencies in README
  • Vignette(s) demonstrating major functionality that runs successfully locally
  • 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).

Functionality

  • 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.
  • Packaging guidelines: The package conforms to the rOpenSci packaging guidelines

Estimated hours spent reviewing: 4


Review Comments

The Rppo package allows programmatic access to the global Plant Phenology (PPO) data portal. This is a compact but well put together package that will come useful to many plant ecologists and evolutionary biologists. Note that although both ppo_data() and ppo_terms() have mostly worked smoothly, I have also experienced some server-related issues while running them. I have provided details below as well as a few other comments/suggestions.

Documentation

I have had difficulties with accessing the rppo vignette from R. This is the relevant devtools::check() output:

* checking files in ‘vignettes’ ... WARNING
Files in the 'vignettes' directory but no files in 'inst/doc':
  ‘rppo-vignette.Rmd’, ‘rppo-vignette.md’
Files named as vignettes but with no recognized vignette engine:
   ‘vignettes/rppo-vignette.Rmd’
(Is a VignetteBuilder field missing?)

Similarly, browseVignettes() fails to find the document.

> browseVignettes("rppo")
No vignettes found by browseVignettes("rppo")

For now I haven't checked the vignette box in the checklist but will do as soon as this is resolved.

In terms of content, the vignette could include a more fleshed out example of how the two functions can be combined to produce additional insights. I imagine they will often be used together?

Functions

  • At times ppo_terms(absent = TRUE) returns:
Error in ppo_terms(absent = TRUE) : uncaught status code  502

On a couple of occasions I have also received this message trying to run the example in the ppo_data() help. Over the span of the 3 separate days I have spent time on the review, both functions have worked absolutely fine in most cases, and I believe this is a temporary issue linked to the queried server. The 502 code corresponds to a failed request where one server receives an invalid response from another server. This makes me think that it would be useful for both functions to return the http status code of each query with the corresponding definition. Were an issue (server or otherwise) to crop up again this would both reassure the user that the function itself is not broken and point to the faulty process.

  • A possible improvement to ppo_data() would be to implement a vectorised version where the function can take a vector of multiple genera as argument. Another option - though this is constrained by what the PPO portal API itself allows - would be to allow for larger taxonomic groups such as orders and families. I would imagine users (I know this is my personal preference) would like receiving all the data they are interested in "in one go", rather than having to write code to go through a list/vector one element at a time.

  • Both ppo_data() and ppo_terms() return data.frames. Given the popularity of tidyverse packages, would it make sense to make them return tibbles by default?

Tests

goodpractice::gp() suggests a couple of small changes:

  • a greater code coverage of unit tests (currently 77% of code lines are covered).
  • considering shortening individual lines of script to remain within 80 characters.

A lone typo (well done!) found by devtools::spell_check(): in ppo_terms.Rd row 20 "pouplates" instead of "populates".

I hope this is useful, thank you very much for your work on this!

@tdjames1
Copy link

tdjames1 commented May 7, 2018

Likewise, here's mine!

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

  • 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
  • Installation instructions: for the development version of package and any non-standard dependencies in README
  • Vignette(s) demonstrating major functionality that runs successfully locally
  • 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).
For packages co-submitting to JOSS

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

  • A short summary describing the high-level functionality of the software
  • Authors: A list of authors with their affiliations
  • 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

  • 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.
  • 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: 5


Review Comments

rppo provides a method of accessing a web data portal to query records of plant phenology observations from North America and Europe. It will be of use to scientists who wish to access global data on plant phenology. While the package is simple to use, it would benefit from amendments to its documentation to clarify its purpose and usage. In particular:

  • README needs examples
  • Vignette could be a fuller description of workflow
  • More examples required for ppo_data function.

I have detailed below a few further points with reference to the ROpenSci package guidelines.


README
  • Add link for "global plant phenology data portal".
  • Use consistent terminology (is it "global plant phenology data portal" or "PPO global data portal"?).
  • Add brief demonstration usage.
  • Add peer review badge?
DESCRIPTION
  • Automated checking tools returned:

checking DESCRIPTION meta-information ... WARNING
Dependence on R version ‘3.4.2’ not with patchlevel 0

Do you need to depend on this particular patchlevel?
From https://cran.r-project.org/doc/manuals/r-patched/R-exts.html#Package-Dependencies:

It is inadvisable to use a dependence on R with patchlevel (the third digit) other than zero. Doing so with packages which others depend on will cause the other packages to become unusable under earlier versions in the series, and e.g. versions 3.x.1 are widely used throughout the Northern Hemisphere academic year.

DOCUMENTATION

rppo

  • Package title in package level documentation doesn't match title in description; suggest using "Access the Global Plant Phenology Data Portal" for the title throughout as this is concise and informative.
  • I was confused by the use of Global Plant Phenology vs Plant Phenology Ontology -- the documentation seems to suggest that these are synonymous but it could be made clearer in the package documentation.
  • The description currently in the documentation for ppo_data should appear here.
  • "Both functions return data frames" is misleading: ppo_data returns a list.

ppo_terms

Description

I suggest keeping the description concise, e.g. "Retrieve terms from the Plant Phenology Ontology", and moving the rest of the information to a "Details" section.

Fix typo: pouplates

ppo_data

Description

This section should describe the function specifically. The paragraph provided is background information that could go in a "Details" section and/or in the package documentation.

Arguments

termID - suggest cross referencing ppo_terms; "to get the relevant IDs for present and absent stages" is not very clear without a fuller description of the function.
fromYear/toYear/fromDay/toDay - indicate parameter format (numeric?); maybe reword, e.g. "return data starting from the specified year".
bbox - the argument description here is too long and unclear. I suggest that the information for creating the bounding box is clarified and moved to a "Details" section of the function documentation. Also, indicate that it's a string value.

Value

I suggest describing the return value explicitly, e.g.

A list containing the following components:
`data`      [describe data component]
`readme`    [etc]
`citation`

Fix typo: resultset

Vignette

  • "A critical element of querying the PPO Data Portal is understanding the present and absent value terms contained in the PPO." Maybe need to explain this if it's critical?
  • Fix typo: 'lis'
  • Include example of results returned from ppo_terms? This could lead into the example for ppo_data, i.e. you could show how the user would inspect the results to find the termID of interest for use in the database query.
  • Place function/object names in back quotes so that they appear as "code"?
  • The "readme" string refers reader to citation_and_data_use_policies.txt - it might be worth clarifying either here or in the function documentation that the citation element contains that information.
  • Package checking tools return:

Files in the 'vignettes' directory but no files in 'inst/doc':
‘rppo-vignette.Rmd’, ‘rppo-vignette.md’

NEWS
  • Include date of release in header line?
TESTING

ppo_data

  • Include tests for alternative query terms e.g. bbox.
  • Include tests for failure to return a result.
  • N.B. exception "uncaught status code" can't be tested for since it is intended to handle an unexpected API response.

ppo_terms

  • Include tests for alternative arguments (present or absent or present and absent).
  • N.B. exception "uncaught status code" can't be tested for since it is intended to handle an unexpected API response.
EXAMPLES

ppo_data

  • Add extra examples to show use of e.g. termID, bbox?
PACKAGE DEPENDENCIES
  • Dependencies seem to be handled appropriately -- only specific functions are imported.
SCAFFOLDING
  • rjson is used to parse JSON. From ROpenSci Packaging Guide:

For parsing JSON, use jsonlite instead of rjson

CONSOLE MESSAGES
  • print is used to report request status. From ROpenSci Packaging Guide:

Use message() and warning() to communicate with the user in your functions. Please do not use print() or cat() unless it's for a print.*() method, as these methods of printing messages are harder for the user to suppress.

CRAN GOTCHAS
  • Make sure your package title is in Title Case.
  • Make sure you include links to websites if you wrap a web API, scrape data from a site, etc. in the Description field of your DESCRIPTION file.
Coding style
  • Code comments for if/else clauses inside relevant block.
  • Check for correct indentation niggles e.g. line 162.
  • Consistent spacing around function parameters and assignment operator.
Code duplication

Not applicable.

User interface issues

Should the data component returned from ppo_data include information about the termIDs associated with the individual observations?

Performance issues

No performance issues noted.

BUGS

ppo_data.R line 115/117 :<- should be :<=

@annakrystalli
Copy link
Contributor

Thanks @remsamp & @tdjames1! 🙌

@jdeck88 both reviews are now in for your consideration. 👍

@annakrystalli
Copy link
Contributor

annakrystalli commented May 8, 2018

@jdeck88 I had also forgotten to ask you to add the rOpenSci review badge to the README! 😬
Could I ask to do this now, please? markdown snippet attached below.

[![](https://badges.ropensci.org/175_status.svg)](https://github.com/ropensci/onboarding/issues/207)

@jdeck88
Copy link
Author

jdeck88 commented May 8, 2018

Thank you for the reviews! I will go through these in the next week.

@jdeck88
Copy link
Author

jdeck88 commented May 17, 2018

Thank you for your reviews. They were very helpful! I have addressed your comments... In most cases i was able to make updates, which are listed under UPDATES below. In some cases i had comments or questions and i've listed these under QUESTIONS/COMMENTS.

UPDATES:

Reviewer @remsamp

  • Fixed vignettes so they are installed in “inst/doc”… this passes the devtools::check(). (however, see my question about the browseVignettes() function in the Questions/Comments.
  • Fixed issue with occassional status code 502 being returned by server. To address this, I implemented two fixes:
    • I inserted more helpful text for the user with a statement that says "Ooops! The server encountered an issue processing your request and returned status code = ",results$status_code,". If the problem persists contact the author."))
    • The server indeed had some issues which were triggered by the testing. I fixed some issues relating to memory management in Node, and am still looking at how the server behaves under stress. However, i have noticed a marked improvement in behavior.
  • Wrote additional tests. Still not 100% coverage since remaining tests cannot be tested for since they rely on an invalid response from the server
  • Shortened all lines of script to remain within 80 characters
  • Typo/spelling issue fixed -pouplates to populates

Reviewer @tdjames1

  • README
    • Added examples
    • Added links where suggested
    • Used consistently terminology (and applied throughout package),
    • Added demonstration usage,
    • Added “in review” ropensci review badge.
  • Description: Changed requirement of R from 3.4.2 to 3.4.0
  • rppo package information:
    • title to match title in description
    • clarified PPO vs. global plant phenology data portal
    • updated description field and put more details in details section
    • updated information on the return value of ppo_data
  • ppo_terms function
    • made description concise and moved details to details section
    • fixed typo
  • ppo_data function
    • Added an additional example
    • Reworded parameter definitions and added return types per suggestions
    • Created a bulleted list displaying return values
  • Vignette:
    • Fixed typos
    • Removed wording describing “critical” and re-phrased.
    • Placed function/object names in back quotes
    • Added an example of results returned from ppo_terms and ppo_data working together
    • Clarified that the citation element contains the information referenced in the “readme” string.
    • Build scripts now placing vignettes files in inst/doc, fixes warning on devtools::check(), however see question in “Comments/Questions” below.
  • NEWS: included date of release in header line… for now, I put May 8th, 2018
  • TESTING:
    • ppo_data
      • added tests for alternate query terms.
      • Included failure return results
      • added a status code from server as return value and test for that
    • ppo_terms
      • included testss for alternative arguments
  • Scaffolding: Used jsonlite instead of rjson
  • Messages: used message() instead of print() to communicate with user
  • CRAN GOTCHAS: made package title in Title Case
  • Coding Style:
    • Made sure if/else clause comments within relevant block
    • Corrected indentation
    • Corrected spacing around function parameter and assignment operators.
  • User Interface issues: Added termIDs to the data component returned from ppo_data: This was not a feature I was planning on implementing until later (for a couple of technical reasons), however, since this seems important now, I went ahead and implemented returning termIDs on the server side and have updated documentation accordingly, including adding a vignette example showing how to work with termIDs.
  • Bugs: fixed R line 115/117 :<- should be :<=

QUESTIONS/COMMENTS:

Reviewer @remsamp

  • browseVignettes(): I spent a while on this and could not get browseVignettes(“rppo”) to work consistently. Also, I’m a little confused by where the resulting vignette files are supposed to reside. Devtools::check() looks for vignettes in inst/doc while browseVignettes seems to looks for them in the vignettes directory off of the source root. As a solution, I installed the generated vignette files in both directories and am including generating both in my build process.
  • Concerning a suggestion to implement a vectorised version where the function can take a vector of multiple genera as argument: Since the Portal contains tens of millions observations and some individual genera could contain possibly close to 1 million records, the design of the portal is to purposefully constrain requests to facilitate response times and predictable behavior. Thus, even though this is possible on the R package end, the portal side will not have it :(
  • Concerning suggestion to use tibbles by default: My personal preference would be to keep imports as slim as possible and deal with a more common return type.

Reviewer @tdjames1 :

  • Per suggestion regarding bbox definition: I borrowed this description from the AntWeb R package which to me was super useful in structuring a bbox query. Moving this to the details section of the function documentation will be less clear IMO. However, I added the notation that it is a string value
  • Per suggestion to add a link in the Description field of your DESCRIPTION file: I attempted adding a link to a website in the Description Field in the DESCRIPTION file but it complained about a malformed sentence. When I remove the link it works.

@annakrystalli
Copy link
Contributor

Thanks for your response @jdeck88!

@tdjames1, @remsamp please let us know if there any further comments/responses to @jdeck88's response or whether you're happy that all issues raised have been appropriately dealt with.

@tdjames1
Copy link

Hi @jdeck88, thanks for the thorough response. The documentation is much improved and the vignette is more helpful now, I think. You could include a full reference to your paper in the vignette rather than just a link (see https://rmarkdown.rstudio.com/authoring_bibliographies_and_citations.html).

Running goodpractice::gp() threw up a couple of further points that have popped up in things that have been changed. I'm flagging these up here as you may wish to address them before submitting to CRAN:

 ✖ use '<-' for assignment instead of '='. '<-' is the standard,
    and R users and developers are used it and it is easier to read your
    code for them if you use '<-'.

    tests/testthat/test-all.R:55:21
    tests/testthat/test-all.R:56:20
    tests/testthat/test-all.R:57:17

  ✖ 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/ppo_data.R:209:45

There still seems to be a typo in ppo_terms.R: line 8 "pouplates"; also I noticed a typo in the updated README.md: line 38 "reccomend".

Regarding URLs in the description field of DESCRIPTION file:

URLs should be enclosed in angle brackets, e.g. ‘<https://www.r-project.org>’: see also Specifying URLs.

I don't know how to address the issues regarding building vignettes, maybe @annakrystalli can advise?

@annakrystalli: pending the few points above I'm satisfied that all issues have been addressed. Do I need to do anything further to sign this off?

@jdeck88
Copy link
Author

jdeck88 commented May 24, 2018

Responses inline and indicated in BOLD (with fixes committed to https://github.com/biocodellc/rppo)

You could include a full reference to your paper in the vignette rather than just a link (see https://rmarkdown.rstudio.com/authoring_bibliographies_and_citations.html).

I tried this and was able to render this in HTML but not in Markdown. I worked on this for awhile this afternoon but felt that a single reference in this simple vignette was not worth the effort to track down why the markdown version was not rendering properly so left the link

Running goodpractice::gp() threw up a couple of further points that have popped up in things that have been changed. I'm flagging these up here as you may wish to address them before submitting to CRAN:

✖ use '<-' for assignment instead of '='. '<-' is the standard,
and R users and developers are used it and it is easier to read your
code for them if you use '<-'.

tests/testthat/test-all.R:55:21
tests/testthat/test-all.R:56:20
tests/testthat/test-all.R:57:17

Fixed

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

Fixed

R/ppo_data.R:209:45

There still seems to be a typo in ppo_terms.R: line 8 "pouplates"; also I noticed a typo in the updated README.md: line 38 "reccomend".

Fixed

Regarding URLs in the description field of DESCRIPTION file:

URLs should be enclosed in angle brackets, e.g. ‘https://www.r-project.org’: see also Specifying URLs.

Ok, i figured that out

@remsamp
Copy link

remsamp commented May 30, 2018

Hi @jdeck88, apologies for the long time in getting back to you. I am happy with your responses to my initial comments, thank you! I can also confirm that the points raised by @tdjames1 have been addressed by your most recent commits.

To note: browseVignettes("rppo") now works after installing the package from github with devtools::install_github("biocodellc/rppo").

@annakrystalli: I am happy with the state the package has got to. Please do let me know if I need to do anything else. Thanks!

@annakrystalli
Copy link
Contributor

annakrystalli commented May 31, 2018

Right, so that's a wrap! Thanks for all your work and input @tdjames1 & @remsamp!

@jdeck88, there's just a few last housekeeping steps to finalise the review process:

  • Transfer the package to the ropensci organisation. Here's the relevant gh help page. However, this might not work because of restricted admin rights. If it doesn't, please transfer the repo to @maelle. She will then transfer it to ropensci and return write rights to you. Feel free to reach out here or to @maelle if you need any help with this.

  • Add the rOpenSci badge to the bottom of the README
    [![ropensci_footer](http://ropensci.org/public_images/github_footer.png)](http://ropensci.org)

  • Change any needed links, such those for CI badges in the README, to redirect to the new repository URL

Any further questions, let me know!

@maelle
Copy link
Member

maelle commented May 31, 2018

👋 @jdeck88 I promise I'd transfer the repo and not keep it to myself! Sorry about the awkward worfklow.

@jdeck88
Copy link
Author

jdeck88 commented May 31, 2018 via email

@maelle
Copy link
Member

maelle commented May 31, 2018

@jdeck88 your repo is now in ropensci and I made you admin again 🎊 thanks for your trust 😉

@annakrystalli
Copy link
Contributor

Great @jdeck88! Thanks again for submitting. I hope you found the process constructive and congratulations on your newly minted rOpenSci package! 🙌

@jdeck88
Copy link
Author

jdeck88 commented Jun 1, 2018

Thanks Anna...
i found the process constructive!

one last thing... is there something i need to change the badge from "in review" to "peer reviewed"?

@annakrystalli
Copy link
Contributor

annakrystalli commented Jun 2, 2018

Good catch @jdeck88. My understanding was that this should update automatically when the issue is closed. @karthik any advice?

@jdeck88
Copy link
Author

jdeck88 commented Jun 4, 2018

Figured this out... turns out i had the wrong issue # in the badge status img link (but the correct one for the hyperlink!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants