-
-
Notifications
You must be signed in to change notification settings - Fork 104
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
rredlist: 'IUCN' Red List Client #663
Comments
Thanks for submitting to rOpenSci, our editors and @ropensci-review-bot will reply soon. Type |
🚀 Editor check started 👋 |
Checks for rredlist (v0.7.1.9000)git hash: 4fd92778
Package License: MIT + file LICENSE 1. Package DependenciesDetails of Package Dependency Usage (click to open)
The table below tallies all function calls to all packages ('ncalls'), both internal (r-base + recommended, along with the package itself), and external (imported and suggested packages). 'NA' values indicate packages to which no identified calls to R functions could be found. Note that these results are generated by an automated code-tagging system which may not be entirely accurate.
Click below for tallies of functions used in each package. Locations of each call within this package may be generated locally by running 's <- pkgstats::pkgstats(<path/to/repo>)', and examining the 'external_calls' table. baselist (56), all (40), paste (24), parse (14), class (3), order (3), c (2), drop (2), if (2), is.null (2), lapply (2), subset (2), any (1), args (1), Filter (1), names (1), Negate (1), Sys.getenv (1), url (1), vapply (1) utilspage (128) rredlistrr_GET (47), check_key (3), ct (2), rl_actions_ (2), rl_categories_ (2), rl_class_ (2), rl_comp_groups_ (2), rl_countries_ (2), rl_extinct_ (2), rl_extinct_wild_ (2), rl_family_ (2), rl_faos_ (2), rredlist_ua (2), assert_is (1), assert_n (1), assert_not_na (1), combine_assessments (1), err_catcher (1), page_assessments (1), release_questions (1), rl_actions (1), rl_api_version (1), rl_assessment (1), rl_assessment_ (1), rl_categories (1), rl_citation (1), rl_class (1), rl_common_names (1), rl_common_names_ (1), rl_comp_groups (1), rl_countries (1), rl_extinct (1), rl_extinct_wild (1), rl_family (1), rl_faos (1), rl_green (1), rl_green_ (1), rl_growth_forms_ (1), rl_habitats_ (1), rl_kingdom_ (1), rl_order_ (1), rl_parse (1), rl_phylum_ (1), rl_pop_trends_ (1), rl_realms_ (1), rl_research_ (1), rl_scopes_ (1), rl_sis (1), rl_species (1), rl_stresses_ (1), rl_systems_ (1), rl_threats_ (1), rl_use_and_trade_ (1), rr_base (1), space (1) statsfamily (3) jsonlitefromJSON (1) NOTE: Some imported packages appear to have no associated function calls; please ensure with author that these 'Imports' are listed appropriately. 2. Statistical PropertiesThis 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:
Statistical properties of package structure as distributional percentiles in relation to all current CRAN packages
All parameters are explained as tooltips in the locally-rendered HTML version of this report generated by the The final measure (
2a. Network visualisationClick to see the interactive network visualisation of calls between objects in package 3.
|
id | name | conclusion | sha | run_number | date |
---|---|---|---|---|---|
11183436924 | lint.yaml | success | 4fd927 | 43 | 2024-10-04 |
11183436931 | R-CMD-check.yaml | success | 4fd927 | 47 | 2024-10-04 |
11183436929 | revdep-check | success | 4fd927 | 38 | 2024-10-04 |
11183436923 | test-coverage.yaml | success | 4fd927 | 43 | 2024-10-04 |
3b. goodpractice
results
R CMD check
with rcmdcheck
rcmdcheck found no errors, warnings, or notes
Test coverage with covr
Package coverage: 94.4
Cyclocomplexity with cyclocomp
No functions have cyclocomplexity >= 15
Static code analyses with lintr
lintr found no issues with this package!
Package Versions
package | version |
---|---|
pkgstats | 0.1.6.17 |
pkgcheck | 0.1.2.58 |
Editor-in-Chief Instructions:
This package is in top shape and may be passed on to a handling editor
I should mention that |
@ropensci-review-bot assign @robitalec as editor |
Assigned! @robitalec is now the editor |
Editor checks:
Editor commentsThanks for the submission @willgearty, I am keen to help on this review! All above looks good to me. Some minor comments below, the authors could consider working on while we are looking for reviewers.
|
@ropensci-review-bot seeking reviewers |
Please add this badge to the README of your package repository: [![Status at rOpenSci Software Peer Review](https://badges.ropensci.org/663_status.svg)](https://github.com/ropensci/software-review/issues/663) 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 |
Oh @willgearty, one more thing. I missed this in the editor checks under Statistical Properties. I see doclines_per_fn_not_exp ("median number of lines of documentation for each non-exported R function") is 0. Looking through your R/ directory, I found these non-exported R functions without any documentation. release.R:
zzz.R:
I would suggest adding some internal documentation for anyone trying to debug an issue when using {rredlist}, or to help potential contributors understand how {rredlist} works. You can use the |
@robitalec thanks for flagging those issues. I believe I have addressed all of your concerns in preparation for review! |
Thank you @willgearty for addressing those suggestions, it looks great! (Sidebar, that is a neat custom chunk option |
@ropensci-review-bot assign @KevCaz as reviewer |
@KevCaz added to the reviewers list. Review due date is 2024-11-14. Thanks @KevCaz for accepting to review! Please refer to our reviewer guide. rOpenSci’s community is our best asset. We aim for reviews to be open, non-adversarial, and focused on improving software quality. Be respectful and kind! See our reviewers guide and code of conduct for more. |
@ropensci-review-bot assign @stephhazlitt as reviewer |
@stephhazlitt added to the reviewers list. Review due date is 2024-11-19. Thanks @stephhazlitt for accepting to review! Please refer to our reviewer guide. rOpenSci’s community is our best asset. We aim for reviews to be open, non-adversarial, and focused on improving software quality. Be respectful and kind! See our reviewers guide and code of conduct for more. |
@stephhazlitt: If you haven't done so, please fill this form for us to update our reviewers records. |
Thanks to both @stephhazlitt and @KevCaz for agreeing to review {rredlist}! Please let me know if you need anything as you work on your review. |
Thank you both in advance for your reviews! |
Package ReviewPlease check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
DocumentationThe package includes all the following forms of documentation:
Functionality
Estimated hours spent reviewing: 7
Review CommentsIn my option this new version of the I haven't identified any major issues in the base code, only suggestions, which I've categorized as 'Should Have' and 'Could Have' below. Please consider the 'Should Have' items as recommendations to address while you're actively working on the package, and the 'Could Have' items as optional improvements. Note that I have submitted a PR (ropensci/rredlist#60) that fixes a few typos and avoid the use of Should HaveAdd a progress bar when there are data to retrieve from more than one pageSome requests require retrieving data from multiple pages, and a dot is printed for each page using If I am not mistaking, there is a simple way to do that. In <crul response>
url: https://api.iucnredlist.org/api/v4/research/1_1?page=1
request_headers:
User-Agent: r-curl/5.2.3 crul/1.5.0 rOpenSci(rredlist/0.7.1.9000)
Accept-Encoding: gzip, deflate
Accept: application/json, text/xml, application/xml, */*
Authorization: L5rzX4M6mJHoH4RFYUxxVLLDorEJ7TTx5ktS
response_headers:
status: HTTP/2 200
cache-control: max-age=0, private, must-revalidate
content-type: application/json
current-page: 1
etag: W/"057a8a5a91bd3e7b59ce06ecde0e7ee1"
link: <https://api.iucnredlist.org/api/v4/research/1_1?page=1&per_pa
ge=
100>; rel="first", <https://api.iucnredlist.org/api/v4/research/1_1?page
=2&
per_page=100>; rel="next", <https://api.iucnredlist.org/api/v4/research/
1_1
?page=275&per_page=100>; rel="last"
page-items: 100
total-count: 27426
total-pages: 275
x-request-id: 7aa92fbc-e51d-4449-96b9-0e6c10b2c9d4
x-runtime: 0.385047
content-length: 23573
date: Fri, 08 Nov 2024 10:26:00 GMT
params:
page: 1
status: 200
you would then be able to use the elements of the response headers (response_headers$ One final thought: This made me consider the possibility of integrating page_assessments() directly within rr_GET(), since it is possible to loop over Second vignetteThe I believe this would be a great way to showcase what can be accomplished with this package and how to integrate it with other packages to answer specific questions. It could also serve as an opportunity to demonstrate how to work with complementary packages or to list blog posts and articles that use Could havetestsThere are tests for all functions to ensure they fail gracefully if the input is not in the correct format (see below for an example of this). However, I'm not sure if this level of testing is strictly necessary. Personally, I'd prefer to see the test_that("fails well", {
skip_on_cran()
expect_error(rl_categories(5), "code must be of class character")
expect_error(rl_categories(list()), "code must be of class character")
expect_error(rl_categories(key = 5), "key must be of class character")
expect_error(rl_categories(key = matrix()), "key must be of class character")
expect_error(rl_categories(parse = 5), "parse must be of class logical")
expect_error(
rl_categories(parse = matrix()),
"parse must be of class logical"
)
expect_error(rl_categories(page = "next"), "page must be of class integer")
expect_error(rl_categories(all = "yes"), "all must be of class logical")
expect_error(rl_categories(quiet = "no"), "quiet must be of class logical")
# lengths
expect_error(rl_categories(page = 1:2), "page must be length 1")
}) Could have
|
Thank you @KevCaz for your review. Just a minor note, I see under "Could have" / "tests" two paragraphs with a few repeated sentences. When you have a moment, can you double check and possibly edit to ensure your comments about tests are clear for everyone? |
@ropensci-review-bot submit review #663 (comment) time 7 |
Couldn't find entry for KevCaz in the reviews log |
Thank you, @robitalec! There were two versions of the same comment, I kept only one version. |
@ropensci-review-bot submit review #663 (comment) time 7 |
Couldn't find entry for KevCaz in the reviews log |
@ropensci-review-bot remove @KevCaz from reviewers |
@KevCaz removed from the reviewers list! |
@ropensci-review-bot assign @KevCaz as reviewer |
@KevCaz added to the reviewers list. Review due date is 2024-12-05. Thanks @KevCaz for accepting to review! Please refer to our reviewer guide. rOpenSci’s community is our best asset. We aim for reviews to be open, non-adversarial, and focused on improving software quality. Be respectful and kind! See our reviewers guide and code of conduct for more. |
So it was a glitch, sorry @KevCaz you are now properly registered! |
@ropensci-review-bot submit review #663 (comment) time 7 |
Logged review for KevCaz (hours: 7) |
Thank you @maelle! |
📆 @stephhazlitt you have 2 days left before the due date for your review (2024-11-19). |
|
Thank you @stephhazlitt for your review! |
@ropensci-review-bot submit review #663 (comment) time 4 |
Logged review for stephhazlitt (hours: 4) |
Hi @willgearty, we have the reviews in from @KevCaz and @stephhazlitt (thank you both). Please feel free to ask the reviewers for any clarification needed on their recommendations. Great work on the package, sounds like both reviewers found it useful, intuitive and well structured. Highlighting some recommendations across the reviews:
I will leave it there for you @willgearty to address the reviewers' comments! 🌻 |
Thank you @KevCaz and @stephhazlitt for your thoughtful reviews (and @robitalec for your very helpful summary)! I'll start working on addressing these recommendations as soon as possible. |
Submitting Author Name: William Gearty
Submitting Author Github Handle: @willgearty
Other Package Authors Github handles: (comma separated, delete if none) @sckott, @maelle
Repository: https://github.com/ropensci/rredlist
Version submitted:
Submission type: Standard
Editor: @robitalec
Reviewers: @stephhazlitt, @KevCaz
Archive: TBD
Version accepted: TBD
Language: en
Scope
Please indicate which category or categories from our package fit policies this package falls under: (Please check an appropriate box below. If you are unsure, we suggest you make a pre-submission inquiry.):
Explain how and why the package falls under these categories (briefly, 1-2 sentences):
Package retrieves data from the IUCN Red List.
The IUCN Red List is used by a broad range of users, including biologists, ecologists, conservationists, activists, etc. It has many uses including
There are no other packages, to my knowledge, that perform the same function.
N/A
N/A
pkgcheck
items which your package is unable to pass.The
pkgcheck
package says my package only has 11.2% coverage, but my own coverage checks calculate the coverage to be ~94%.Technical checks
Confirm each of the following by checking the box.
This package:
Publication options
Do you intend for this package to go on CRAN?
Package is already on CRAN.
Do you intend for this package to go on Bioconductor?
Do you wish to submit an Applications Article about your package to Methods in Ecology and Evolution? If so:
MEE Options
Code of conduct
The text was updated successfully, but these errors were encountered: