Closed salvafern closed 3 weeks ago
Thanks for submitting to rOpenSci, our editors and @ropensci-review-bot will reply soon. Type @ropensci-review-bot help
for help.
:rocket:
Editor check started
:wave:
git hash: 9ed0de6d
Package License: MIT + file LICENSE
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.
|type |package | ncalls|
|:----------|:---------------|------:|
|internal |base | 176|
|internal |mregions2 | 121|
|internal |stats | 24|
|imports |magrittr | 200|
|imports |httr2 | 51|
|imports |checkmate | 45|
|imports |methods | 19|
|imports |glue | 18|
|imports |sf | 14|
|imports |cli | 7|
|imports |memoise | 5|
|imports |utils | 3|
|imports |dplyr | 3|
|imports |ISOcodes | 2|
|imports |digest | 2|
|imports |rdflib | 1|
|imports |xml2 | 1|
|imports |wrapr | NA|
|imports |curl | NA|
|suggests |leaflet | 10|
|suggests |ows4R | NA|
|suggests |httptest2 | NA|
|suggests |jsonlite | NA|
|suggests |knitr | NA|
|suggests |leaflet.extras2 | NA|
|suggests |mapview | NA|
|suggests |mregions | NA|
|suggests |rmarkdown | NA|
|suggests |testthat | NA|
|suggests |wk | NA|
|suggests |purrr | NA|
|suggests |withr | NA|
|linking_to |NA | NA|
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(
%>% (200)
c (28), url (11), source (9), tolower (9), for (8), list (8), names (8), subset (8), all (7), as.character (7), unique (7), file.path (5), options (5), paste0 (5), data.frame (4), format (4), length (4), seq_along (4), gsub (3), if (3), version (3), class (2), dim (2), is.null (2), lapply (2), nrow (2), tempdir (2), as.integer (1), by (1), difftime (1), file.info (1), getOption (1), ifelse (1), is.na (1), readLines (1), rm (1), sort (1), substitute (1), Sys.getenv (1), unlist (1), with (1)
mrp_view (14), is_error (11), marineregions.org (9), base_map (3), gaz_records_by_name_at (3), gaz_rest_sources (3), get_records_by_lat_long_at (3), get_records_by_type_at (3), get_source_at (3), gaz_rest_geometries (2), gaz_rest_record_by_mrgid (2), gaz_rest_records_by_lat_long (2), gaz_rest_records_by_names (2), gaz_rest_source_by_sourceid (2), gaz_rest_types (2), gaz_search.sfg (2), add_labels (1), assert_deps (1), assert_internet (1), assert_mrgid_exists (1), assert_only_one_filter (1), assert_placetype (1), assert_service (1), assert_typeid (1), c_rdf (1), cache_max_time (1), check_if_missing (1), check_server_warning (1), gaz_add_geometry (1), gaz_geometry (1), gaz_geometry.mr_df (1), gaz_geometry.numeric (1), gaz_relations (1), gaz_relations.mr_df (1), gaz_relations.numeric (1), gaz_rest_geometry (1), gaz_rest_names_by_mrgid (1), gaz_rest_records_by_name (1), gaz_rest_records_by_source (1), gaz_rest_records_by_type (1), gaz_rest_relations_by_mrgid (1), gaz_rest_wmses (1), gaz_search (1), gaz_search_by_source (1), gaz_search_by_source.character (1), gaz_search_by_source.numeric (1), gaz_search_by_type (1), gaz_search_by_type.character (1), gaz_search_by_type.numeric (1), gaz_search.character (1), gaz_search.numeric (1), gaz_search.sf (1), gaz_search.sfc (1), geom_perform (1), is_internet_down (1), is_test (1), mr_memoise (1), mrp_get (1), mrp_get_sanity_check (1), mrp_view_cds (1), mrp_view_eca_reg13_nox (1), mrp_view_eca_reg14_sox_pm (1), mrp_view_ecoregions (1), mrp_view_ecs (1), mrp_view_ecs_boundaries (1), mrp_view_eez (1), mrp_view_eez_12nm (1), mrp_view_eez_24nm (1), mrp_view_eez_archipelagic_waters (1), mrp_view_eez_boundaries (1), mrp_view_eez_iho (1)
req_headers (12), req_error (11), resp_check_status (8), resp_status (5), req_url_query (4), request (3), url_parse (3), url_build (2), resp_body_string (1), resp_is_error (1), resp_status_desc (1)
assert_character (15), assert_integerish (15), assert_int (4), assert_logical (4), assert_numeric (3), assert_data_frame (2), assert_double (2)
offset (18), filter (5), df (1)
coerce (18), is (1)
glue (18)
st_read (3), st_as_sfc (2), st_bbox (2), st_crs (2), st_point (2), st_sfc (2), st_coordinates (1)
tileOptions (3), leafletCRS (2), leafletOptions (2), addTiles (1), leaflet (1), WMSTileOptions (1)
cli_abort (7)
memoise (5)
right_join (2), bind_rows (1)
unzip (3)
digest (2)
ISO_639_2$Alpha_2 (1), ISO_639_2$Name (1)
rdf_serialize (1)
xml_attr (1)
magrittr
base
mregions2
httr2
checkmate
stats
methods
glue
sf
leaflet
cli
memoise
dplyr
utils
digest
ISOcodes
rdflib
xml2
This package features some noteworthy statistical properties which may need to be clarified by a handling editor prior to progressing.
The package has: - code in R (100% in 12 files) and - 2 authors - 3 vignettes - 1 internal data file - 16 imported packages - 59 exported functions (median 3 lines of code) - 123 non-exported functions in R (median 6 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 All parameters are explained as tooltips in the locally-rendered HTML version of this report generated by [the `checks_to_markdown()` function](https://docs.ropensci.org/pkgcheck/reference/checks_to_markdown.html) 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 | 12| 65.5| | |files_vignettes | 3| 92.4| | |files_tests | 43| 99.0| | |loc_R | 1104| 70.7| | |loc_vignettes | 391| 71.7| | |loc_tests | 1807| 93.2| | |num_vignettes | 3| 94.2| | |data_size_total | 7328| 69.2| | |data_size_median | 7328| 77.0| | |n_fns_r | 182| 88.4| | |n_fns_r_exported | 59| 90.2| | |n_fns_r_not_exported | 123| 87.4| | |n_fns_per_file_r | 10| 87.7| | |num_params_per_fn | 2| 11.9| | |loc_per_fn_r | 5| 8.1| | |loc_per_fn_r_exp | 3| 1.5|TRUE | |loc_per_fn_r_not_exp | 6| 13.8| | |rel_whitespace_R | 35| 85.0| | |rel_whitespace_vignettes | 55| 86.2| | |rel_whitespace_tests | 12| 86.1| | |doclines_per_fn_exp | 55| 68.4| | |doclines_per_fn_not_exp | 0| 0.0|TRUE | |fn_call_network_size | 100| 79.2| | ---
Click to see the interactive network visualisation of calls between objects in package
goodpractice
and other checks#### 3a. Continuous Integration Badges [![R-CMD-check.yaml](https://github.com/lifewatch/mregions2/actions/workflows/R-CMD-check.yaml/badge.svg)](https://github.com/lifewatch/mregions2/actions) [![pkgcheck](https://github.com/lifewatch/mregions2/workflows/pkgcheck/badge.svg)](https://github.com/lifewatch/mregions2/actions) **GitHub Workflow Results** | id|name |conclusion |sha | run_number|date | |----------:|:--------------------------|:----------|:------|----------:|:----------| | 4809625294|pages build and deployment |success |c80782 | 33|2023-04-26 | | 4807668829|pkgcheck |success |9ed0de | 82|2023-04-26 | | 4809552258|pkgdown |success |9ed0de | 49|2023-04-26 | | 4807666863|R-CMD-check |success |9ed0de | 31|2023-04-26 | | 4807668826|R-CMD-check-schedule |success |9ed0de | 122|2023-04-26 | | 4807666847|test-coverage |success |9ed0de | 30|2023-04-26 | --- #### 3b. `goodpractice` results #### `R CMD check` with [rcmdcheck](https://r-lib.github.io/rcmdcheck/) R CMD check generated the following note: 1. checking data for non-ASCII characters ... NOTE Note: found 1 marked UTF-8 string R CMD check generated the following check_fail: 1. rcmdcheck_non_ascii_characters_in_data #### Test coverage with [covr](https://covr.r-lib.org/) Package coverage: 91.54 #### Cyclocomplexity with [cyclocomp](https://github.com/MangoTheCat/cyclocomp) The following functions have cyclocomplexity >= 15: function | cyclocomplexity --- | --- gaz_rest_records_by_name | 19 gaz_add_geometry | 15 #### Static code analyses with [lintr](https://github.com/jimhester/lintr) [lintr](https://github.com/jimhester/lintr) found the following 403 potential issues: message | number of times --- | --- Avoid library() and require() calls in packages | 11 Lines should not be more than 80 characters. | 392
|package |version | |:--------|:--------| |pkgstats |0.1.3.4 | |pkgcheck |0.1.1.23 |
This package is in top shape and may be passed on to a handling editor
Thanks @salvafern for this submission and for pointing to https://github.com/ropensci/mregions/pull/62.
How can we leave a record that the maintainer of mregions is OK with the decision of submitting mregions2? Maybe you can point me to an GH-issue comment?
Hi @maurolepore, thanks for checking out! π
I am actually the current maintainer of mregions (https://github.com/ropensci/mregions/commit/e5fb45be93a957e4ccc51158bb900f72dbcd08a1). Before me were @LennertSchepers and @sckott (https://github.com/ropensci/mregions/commit/dd7b6e44aef74808f3f0f75a09d0568356ce632d).
Since we had this public conversation on https://github.com/ropensci/mregions/pull/62, and also got the idea of a new package from @maelle (see https://github.com/ropensci/mregions/pull/62#issuecomment-1061969979), I think we are all on the same page about submitting mregions2. But if there is any objection from Lennert or Scott I'm happy to discuss of course π
No objection from me
No objection from me, @salvafern is in our team so we discussed the whole process in detail.
Worth noting if you need written approval: we are also the Marine Regions data system developers/maintainers/owners (see e.g. https://marineregions.org/editors.php)
Thanks @salvafern and everyone else for your support. I'll stat searching for a handling editor.
@ropensci-review-bot assign @jooolia as editor
Assigned! @jooolia is now the editor
[X] Documentation: The package has sufficient documentation available online (README, pkgdown docs) to allow for an assessment of functionality and scope without installing the package. In particular,
[X] Installation instructions: Are installation instructions clear enough for human users?
[X] Tests: If the package has some interactivity / HTTP / plot production etc. are the tests using state-of-the-art tooling?
[ ] Contributing information: Is the documentation for contribution clear enough e.g. tokens for tests, playgrounds?
[X] License: The package has a CRAN or OSI accepted license.
Dear @salvafern and @whaleshark99,
Thanks for your submission. Overall the package is in great shape. The only thing that I would note would that it would be even better with more information on how to contribute to the package. Feel free to let me know if there is a reason that additional info should not be added. I will start looking for reviewers.
Cheers, Julia
@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/590_status.svg)](https://github.com/ropensci/software-review/issues/590)
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
Hello, I am still actively looking for reviewers. Thanks, Julia
@jooolia I accept your invitation to review it. This is my first attempt to review a package. Do you have a Test Case to look for bugs. Do I have to get a data frame to test all those 30 functions/methods
@jooolia I found the guidelines https://devguide.ropensci.org/reviewerguide.html I am sorry for my earlier mail
@ropensci-review-bot assign @Kattuvan as reviewer
@Kattuvan added to the reviewers list. Review due date is 2023-06-20. Thanks @Kattuvan 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.
@Kattuvan: If you haven't done so, please fill this form for us to update our reviewers records.
Thanks @Kattuvan for agreeing to review! Glad that you found the guide. :)
@ropensci-review-bot assign @sheilasaia as reviewer
@sheilasaia added to the reviewers list. Review due date is 2023-06-21. Thanks @sheilasaia 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.
@sheilasaia: If you haven't done so, please fill this form for us to update our reviewers records.
Thanks @sheilasaia for agreeing to review. Please feel free to reach out if you have any questions. Thanks, Julia
Hi Julia, Please find below the results I obtained using the 'pkgcheck'Β on 'mregions2'. Are you expecting me to use the package against the data for further check. This is my first review of any package. I appreciate your guidance. Thanks Murali.
summary(x)
ββ mregions2 1.0.0 ββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββ
β Package name is available β has a 'codemeta.json' file. β has a 'contributing' file. β uses 'roxygen2'. β 'DESCRIPTION' has a URL field. β 'DESCRIPTION' has a BugReports field. β Package has at least one HTML vignette β All functions have examples. β Package has no continuous integration checks. β Package coverage is 91.5%. β R CMD check found 1 error. β R CMD check found 1 warning.
βΉ Current status: β This package is not ready to be submitted.
print(x)
ββ mregions2 1.0.0 ββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββ
β Package name is available β has a 'codemeta.json' file. β has a 'contributing' file. β uses 'roxygen2'. β 'DESCRIPTION' has a URL field. β 'DESCRIPTION' has a BugReports field. β Package has at least one HTML vignette β All functions have examples. β Package has no continuous integration checks. β Package coverage is 91.5%. β R CMD check found 1 error. β R CMD check found 1 warning.
βΉ Current status: β This package is not ready to be submitted.
ββ git ββ
β’ HEAD: 9ed0de6d β’ Default branch: main β’ Number of commits: 134 β’ First commit: 27-04-2022 β’ Number of authors: 3
ββ Package Structure ββ
βΉ Package uses the following languages: β’ : %
βΉ Package has β’ 2 authors. β’ 3 vignettes. β’ 1 internal data file. β’ 16 imported packages. β’ 59 exported functions (median 3 lines of code). β’ 123 non-exported functions (median 6 lines of code). β’ 2 parameters per function (median).
ββ Package statistics ββ
measure value percentile noteworthy
1 files_R 0.0 0.0 TRUE
4 files_vignettes 0.0 0.0 TRUE
5 files_tests 0.0 0.0 TRUE
6 num_vignettes 3.0 94.2
7 data_size_total 7328.0 69.2
8 data_size_median 7328.0 77.0
9 n_fns_r 182.0 88.4
10 n_fns_r_exported 59.0 90.2
11 n_fns_r_not_exported 123.0 87.4
12 n_fns_per_file_r 10.2 87.7
13 num_params_per_fn 2.0 11.9
14 loc_per_fn_r 5.0 8.1
15 loc_per_fn_r_exp 3.0 1.5 TRUE
16 loc_per_fn_r_not_exp 6.0 13.8
17 doclines_per_fn_exp 55.0 68.4
18 doclines_per_fn_not_exp 0.0 0.0 TRUE
19 fn_call_network_size 100.0 79.2
βΉ Package network diagram is at [C:/Users/Murali/AppData/Local/Cache/R/pkgcheck/static/mregions2_pkgstats9ed0de6d.html].
ββ goodpractice ββ
ββ GP mregions2 βββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββ
It is good practice to
β write unit tests for all functions, and all package code in general. 91% of code lines are covered by test cases.
R/00_gaz_geom.R:54:NA
R/00_gaz_geom.R:338:NA
R/00_gaz_geom.R:339:NA
R/00_gaz_geom.R:340:NA
R/00_gaz_geom.R:341:NA
... and 71 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
data-raw\mrp_list.R:20:81
R\00_gaz_geom.R:6:81
R\00_gaz_geom.R:9:81
R\00_gaz_geom.R:10:81
R\00_gaz_geom.R:13:81
... and 387 more lines
β fix this R CMD check NOTE: Note: found 1 marked UTF-8 string β fix this R CMD check WARNING: LaTeX errors when creating PDF version. This typically indicates Rd problems. βββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββ ββ Package Versions ββ
pkgstats: 0.1.3.4 pkgcheck: 0.1.1.24
results_of_x <- print(x)
ββ mregions2 1.0.0 ββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββ
β Package name is available β has a 'codemeta.json' file. β has a 'contributing' file. β uses 'roxygen2'. β 'DESCRIPTION' has a URL field. β 'DESCRIPTION' has a BugReports field. β Package has at least one HTML vignette β All functions have examples. β Package has no continuous integration checks. β Package coverage is 91.5%. β R CMD check found 1 error. β R CMD check found 1 warning.
βΉ Current status: β This package is not ready to be submitted.
ββ git ββ
β’ HEAD: 9ed0de6d β’ Default branch: main β’ Number of commits: 134 β’ First commit: 27-04-2022 β’ Number of authors: 3
ββ Package Structure ββ
βΉ Package uses the following languages: β’ : %
βΉ Package has β’ 2 authors. β’ 3 vignettes. β’ 1 internal data file. β’ 16 imported packages. β’ 59 exported functions (median 3 lines of code). β’ 123 non-exported functions (median 6 lines of code). β’ 2 parameters per function (median).
ββ Package statistics ββ
measure value percentile noteworthy
1 files_R 0.0 0.0 TRUE
4 files_vignettes 0.0 0.0 TRUE
5 files_tests 0.0 0.0 TRUE
6 num_vignettes 3.0 94.2
7 data_size_total 7328.0 69.2
8 data_size_median 7328.0 77.0
9 n_fns_r 182.0 88.4
10 n_fns_r_exported 59.0 90.2
11 n_fns_r_not_exported 123.0 87.4
12 n_fns_per_file_r 10.2 87.7
13 num_params_per_fn 2.0 11.9
14 loc_per_fn_r 5.0 8.1
15 loc_per_fn_r_exp 3.0 1.5 TRUE
16 loc_per_fn_r_not_exp 6.0 13.8
17 doclines_per_fn_exp 55.0 68.4
18 doclines_per_fn_not_exp 0.0 0.0 TRUE
19 fn_call_network_size 100.0 79.2
βΉ Package network diagram is at [C:/Users/Murali/AppData/Local/Cache/R/pkgcheck/static/mregions2_pkgstats9ed0de6d.html].
ββ goodpractice ββ
ββ GP mregions2 βββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββ
It is good practice to
β write unit tests for all functions, and all package code in general. 91% of code lines are covered by test cases.
R/00_gaz_geom.R:54:NA
R/00_gaz_geom.R:338:NA
R/00_gaz_geom.R:339:NA
R/00_gaz_geom.R:340:NA
R/00_gaz_geom.R:341:NA
... and 71 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
data-raw\mrp_list.R:20:81
R\00_gaz_geom.R:6:81
R\00_gaz_geom.R:9:81
R\00_gaz_geom.R:10:81
R\00_gaz_geom.R:13:81
... and 387 more lines
β fix this R CMD check NOTE: Note: found 1 marked UTF-8 string β fix this R CMD check WARNING: LaTeX errors when creating PDF version. This typically indicates Rd problems. βββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββ ββ Package Versions ββ
pkgstats: 0.1.3.4 pkgcheck: 0.1.1.24 On Tuesday, May 30, 2023 at 11:37:37 p.m. EDT, Julia Gustavsen @.***> wrote:
Thanks @sheilasaia for agreeing to review. Please feel free to reach out if you have any questions. Thanks, Julia
β Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>
Dear @Kattuvan , Great that you ran {pkgcheck}, will you follow up with the review template ? Here there are specific things to check and an opportunity to provide your comments on the package to the authors (related to the code, the usability and functionality). Thanks, Julia
Hi Julia, Please find attached duly filled review template herewith. Thank you. Murali On Monday, June 5, 2023 at 02:16:43 a.m. EDT, Julia Gustavsen @.***> wrote:
Dear @Kattuvan , Great that you ran {pkgcheck}, will you follow up with the review template ? Here there are specific things to check and an opportunity to provide your comments on the package to the authors (related to the code, the usability and functionality). Thanks, Julia
β Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>
Hi @Kattuvan, thanks for the doing the review. I think you may have attached something via email, but this did not appear on Github. Could you paste your review directly in the issue by logging into Github? Thanks, Julia
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
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
dependencies βsfβ; βdplyrβ; βmagritterβ are referenced elsewhere. https://lifewatch.github.io/mregions2/articles/mregions2.html/
β Vignette(s): demonstrating major functionality that runs successfully locally
Package has at least one HTML vignette
But Vignette is available only through 'READ.md'
β Function Documentation: for all exported functions
β Examples: (that run successfully locally) for all exported functions
Examples donβt run in place.
All functions have examples
β Community guidelines: including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL
, BugReports
and Maintainer
(which may be autogenerated via Authors@R
).
availble
β Installation: Installation succeeds as documented.
Removed βsfβ; βdplyrβ; βmagritterβ from local system to test installation of βmregion2β
I get error message βError in loadNamespace(j <- i[[1L]], c(lib.loc, .libPaths()), versionCheck = vI[[j]]) : there is no package called βmagrittrββ
So, I installed βmagrittrβ before attempting again βmregion2β
βmregions2β installs dependencies βsfβ and βdplyrβ
β 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.
It is good practice to write unit tests for all functions, and all package code in general. 91% of code lines are covered by test cases.
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
β Packaging guidelines: The package conforms to the rOpenSci packaging guidelines.
This package is not ready to be submitted.
Package has no continuous integration checks.
Estimated hours spent reviewing: 16 Hours
Hi Julia, Done it as your requirements. Murali On Friday, June 9, 2023 at 02:38:32 a.m. EDT, Julia Gustavsen @.***> wrote:
Hi @Kattuvan, thanks for the doing the review. I think you may have attached something via email, but this did not appear on Github. Could you paste your review directly in the issue by logging into Github? Thanks, Julia
β Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>
Hi @Kattuvan Thanks for using the template. Do you have any comments overall to accompany your review or any comments on specific lines of code? Thanks, Julia
:calendar: @Kattuvan you have 2 days left before the due date for your review (2023-06-20).
:calendar: @sheilasaia you have 2 days left before the due date for your review (2023-06-21).
Here's my review. Thank you for the time extension.
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
The package includes all the following forms of documentation:
URL
, BugReports
and Maintainer
(which may be autogenerated via Authors@R
).Estimated hours spent reviewing: 4 hours
Overall comments:
This package enables folks to directly access two specific datasets directly in their R session. I'm not familiar with the specific datasets myself, but can see how this would be helpful and a unique contribution to the R package space. I didn't have any problems with installing the package and all the lines of code in the vignette worked well including the leaflet mapping functions. That said, there are several things that I think still need to be addressed before the package can be submitted.
I left some specific comments below. Thanks for the opportunity to review this rOpenSci package submission!
Some specific comments:
gaz_search()
? Specifically it was gaz_search("Belgian Part of the North Sea")
. When I first looked at this function in the vignette I mistook the input as having to only be a country so an example of what you all mean by "free text" would be helpful to see.gaz_rest()
I don't get a help page. I get an error that it cannot find the function: "Error in gaz_rest() : could not find function "gaz_rest"" I tried running without the ()
at the end and am also getting an error.mrp_view()
please explain that all the functions under mrp_view()
are for specific layers in the mrp data file. Maybe include a sub heading such as "Specific Layer Usage" or something similar to that to make this clear.mrp_col_unique()
but it would be helpful to include this since it can give the user a sense of what columns are available to them if they are not as familiar with the structure of the Marine Regions data product. Thinking a bit more broadly, it might also be helpful to give the user a broader sense of the columns of data that will be returned when using functions; however, this could be either added to the function documentation or up at the start of the README file (see my comment number 4).gaz_search_by_type("EEZ")
was taking me 13 seconds to run, which is still fast, but longer than the other functions were taking (i.e., generally most took 1-2 seconds). I wanted to let the authors know this in case that's unexpected.Hi all,
Thank you very much @jooolia for coordinating the reviews; and @Kattuvan and @sheilasaia for your thorough reviews!
I'm currently the solely developer of mregions2 and I'm also going on leave soon till 2023-07-24, thus won't be able to address the reviews until then. Hope this is not a problem, otherwise I will try to find someone who can pick this up during my absence.
Hi @salvafern, Thanks for letting us know. That is fine, talk to you when you are back. Cheers, Julia
Hi @salvafern, Hope that you are well. Do you have any updates on your progress? Thanks, Julia
Hi @jooolia
Yes I am currently addressing the reviews (see https://github.com/lifewatch/mregions2/issues/1#issuecomment-1710318418 , will be updating the comment). Apologies for the delay, I haven't been able to allocate time before. I should be able to do most of the changes this week.
Hi all, thank you for the patience and for all your valuable feedback. I have been tackling most of your comments. Here are some follow-ups. Note I am experiencing problems with the CI (https://github.com/lifewatch/mregions2/issues/21) but it is an issue of pak
when preparing the environment in ubuntu. The R CMD check in ubuntu outside the CI works fine. We hope it will be solved with a future update of pak
.
@jooolia
Editor comments
Dear @salvafern and @whaleshark99,
Thanks for your submission. Overall the package is in great shape. The only thing that I would note would that it would be even better with more information on how to contribute to the package. Feel free to let me know if there is a reason that additional info should not be added. I will start looking for reviewers.
Cheers, Julia
Done: https://github.com/lifewatch/mregions2/commit/de65fbce302d70d16b2c8b94662857cefffea03d
@Kattuvan
* β **Installation instructions:** for the development version of package and any non-standard dependencies in README * dependencies βsfβ; βdplyrβ; βmagritterβ are referenced elsewhere. https://lifewatch.github.io/mregions2/articles/mregions2.html/
These dependencies are listed in Imports. It is true however that the README did not load magrittr
. I have fixed that: https://github.com/lifewatch/mregions2/commit/dcbd99ebe97747020f0be340a5f6750f93e22f23 let me know if it is ok
* β **Vignette(s):** demonstrating major functionality that runs successfully locally * Package has at least one HTML vignette * But Vignette is available only through 'READ.md' * β **Function Documentation:** for all exported functions
I added indeed only one vignette in an strict sense, the rest are online articles via usethis::use_article() cause I did not consider they needed to be in the package (e.g. why_mregions2). Or cause they are experimental work that is prone to errors (e.g. mregions2-rdf).
However the get started vignette is available in the R package. I added the build_vignettes = TRUE
argument in the installation instructions: https://github.com/lifewatch/mregions2/commit/dcbd99ebe97747020f0be340a5f6750f93e22f23
* β **Examples:** (that run successfully locally) for all exported functions * Examples donβt run in place.
Examples are indeed wrapped on \dontrun{}
. This is because mregions2 uses webservices to access the data. These web services may fail once just cause a network temporary issue. In that case the whole check would fail. These same functions are however tested. They are mocked with httptest2
.
I doubled checked with devtools::run_examples(run_dontrun = TRUE)
and they run fine. I can add this to the R-CMD-Check-schedule.yaml
action (explained further below) but at the moment is failing. I will do it when this is solved.
* β **Functionality:** Any functional claims of the software been confirmed. * β **Performance:** Any performance claims of the software been confirmed.
Are these ok?
* β **Packaging guidelines**: The package conforms to the rOpenSci packaging guidelines. * This package is not ready to be submitted. * Package has no continuous integration checks.
The package uses httptest2
to mock up http Reponses.
The normal R-CMD-Check it is indeed only run when something new is added
There is a github action that runs the R-CMD-Check scheduled to run once a week and that tests for real interactions instead of using the mocked HTTP responses. This is how rOpenSci recommends to do testing of http https://books.ropensci.org/http-testing/httptest2.html#httptest2-real
I think this is already fine but I can make the normal R-CMD-Check.yaml to run also periodically.
@sheilasaia
1. I suggest you list the current maintainer on the GitHub page "About" section on the right (https://github.com/lifewatch/mregions2). I don't see that there, currently, but do see it in the DESCRIPTION file.
Done. I didn't see an option on github to add this specifically so I just added my github name to the About section.
2. I see there's an MIT license file (license.md) given but this is not connected to the "About" section on the right of the main GitHub page. I suggest fixing the link to that file and also spelling out the specific author names directly in the license file so that's clear. It's currently listed as "authors".
Done: https://github.com/lifewatch/mregions2/commit/de65fbce302d70d16b2c8b94662857cefffea03d https://github.com/lifewatch/mregions2/commit/78df1ad033a3affc80886ef0fd0b5fb10849dfa4 https://github.com/lifewatch/mregions2/commit/4c1e2243273548b5519ef669ab6b135927cbeb78
4. Can the authors give a little bit more info about the audience of this package in the description at the very top of the readme? Maybe they can take some of the text from this submission and add it there. It would be helpful to have a sentence or two at the very top that directly explains what types of data are in the Marine Regions Gazetteer and Marine Regions Data Products for those that are not familiar with these resources and would like to know this from quickly scanning the readme. If they want to know more then they can follow the external links that were provided.
Done: https://github.com/lifewatch/mregions2/commit/40cdf9939c78b8e0798f9c6bf4d11a93cee44a2d
5. Can the authors use the example in the R script for `gaz_search()`? Specifically it was `gaz_search("Belgian Part of the North Sea")`. When I first looked at this function in the vignette I mistook the input as having to only be a country so an example of what you all mean by "free text" would be helpful to see.
Done: https://github.com/lifewatch/mregions2/commit/6a7b2441a0a4cda712e702b75fe80109fb876d16 Let me know if this suffices or it is still not clear
6. When I run `gaz_rest()` I don't get a help page. I get an error that it cannot find the function: "Error in gaz_rest() : could not find function "gaz_rest"" I tried running without the `()` at the end and am also getting an error.
I cannot reproduce this. gaz_rest
is only documentation so it won't work with brackets. However it renders fine for me and also in the pkgdown website . Can you share a reprex including your sessionInfo()
?
7. In the usage description of `mrp_view()` please explain that all the functions under `mrp_view()` are for specific layers in the mrp data file. Maybe include a sub heading such as "Specific Layer Usage" or something similar to that to make this clear.
Done: https://github.com/lifewatch/mregions2/commit/18696d175cac7e954308f74baebe34e4330bd7a3
8. The authors don't have a vignette for `mrp_col_unique()` but it would be helpful to include this since it can give the user a sense of what columns are available to them if they are not as familiar with the structure of the Marine Regions data product. Thinking a bit more broadly, it might also be helpful to give the user a broader sense of the columns of data that will be returned when using functions; however, this could be either added to the function documentation or up at the start of the README file (see my comment number 4).
Tackling here: https://github.com/lifewatch/mregions2/issues/20 For now I did: https://github.com/lifewatch/mregions2/commit/ca09f8a215c953df5b5bac143080d8848a40e69d
mrp_ontology
We will continue updating the list - in the future we intend to standardize the attributes with controlled vocabularies
9. The line `gaz_search_by_type("EEZ")` was taking me 13 seconds to run, which is still fast, but longer than the other functions were taking (i.e., generally most took 1-2 seconds). I wanted to let the authors know this in case that's unexpected.
Yes, it is not strange behavior :) if the response is heavy it might take a while to load
10. It would be helpful to have a section in the bottom of the README that discusses whether there are similar packages or maybe even complimentary packages to this one. That way the user get get a quick background.
Done: https://github.com/lifewatch/mregions2/commit/e0a571c1ba7de65ad337cb284fc4d16a100ba2c6
@salvafern, thanks for your replies and I think you did a great job addressing them!
I looked back at gaz_rest
and I'm realizing what you said in your comment; it's not a function. I think the documentation was a little confusing to me since it showed up in the documentation help pages but is not, for example, like gaz_rest_geometries()
in that it doesn't have a usage case listed. I was wondering if a help page for gaz_rest
is needed? If you think it is, I would state somewhere that it's not a function and you could consider instead pointing people to a page with a summary of the other functions options (i.e., those that start with gaz_rest_*()
like gaz_rest_geometries()
).
Uploading a screenshot so you can see that when I run ?gaz_rest
.
Hi @sheilasaia, Indeed it was not that clear. The purpose of gaz_rest
was to explain quickly why there are functions with names starting as gaz_rest_*
. I think it is beneficial to keep this doc as I can easily link it to all gaz_rest_*
functions through the section @ seealso . I improved the explanations now (https://github.com/lifewatch/mregions2/commit/a86a1d4ad391bfb96d7f29171056fecda0568779). Let me know if it is better or you would still remove it from the docs.
Looks good to me, @salvafern! Thank you.
Hello @salvafern, sorry for the delay in my handling. I missed these updates. :( Thank you for all of the work!
Hi @Kattuvan, Do you feel that the changes and updates have addressed your concerns? Thanks, Julia
Hi @jooolia, did you receive any message from @Kattuvan ? Any chance to move this submission forward? Best, Salva
Hi,Please move the submission to the next level. Regards,Murali On Thursday, February 22, 2024 at 08:44:27 a.m. EST, salvafern @.***> wrote:
Hi @jooolia, did you receive any message from @Kattuvan ? Any chance to move this submission forward? Best, Salva
β Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>
Hi @jooolia , please let me know if there is anything else I can do on my side to move this submission forward. Best, Salva
Dear Salva,
Sorry for the delay. I will get back to you at the latest by March 10th. Thank you for your patience.
Cheers, Julia
Le lun. 4 mars 2024, 16:02, salvafern @.***> a Γ©crit :
Hi @jooolia https://github.com/jooolia , please let me know if there is anything else I can do on my side to move this submission forward. Best, Salva
β Reply to this email directly, view it on GitHub https://github.com/ropensci/software-review/issues/590#issuecomment-1976783812, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAOPZSXTJENIDT2YUMMWATDYWSEIBAVCNFSM6AAAAAAXMQPDZCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNZWG44DGOBRGI . You are receiving this because you were mentioned.Message ID: @.***>
Dear @salvafern, Things are looking good, although the comments about the CI have not fully been addressed in my view. I had a quick look, but the logs are too old (more than three months) to be viewed. Could you try re-running the Github Actions for the pkg-check and R-CMD-check so that we can help troubleshoot what is happening here?
Thanks!
Date accepted: 2024-06-09
Submitting Author Name: Salvador FernΓ‘ndez Bejarano Submitting Author Github Handle: !--author1-->@salvafern<!--end-author1-- Other Package Authors Github handles: (comma separated, delete if none) !--author-others-->@lottepohl<!--end-author-others-- Repository: https://github.com/lifewatch/mregions2 Version submitted: Submission type: Standard Editor: !--editor-->@jooolia<!--end-editor-- Reviewers: @Kattuvan, @sheilasaia
Due date for @Kattuvan: 2023-06-20 Due date for @sheilasaia: 2023-06-21Archive: TBD Version accepted: TBD Language: en
Scope
This package reads the Marine Regions Gazetteer and the Marine Regions Data Products via a REST API and OGC web services respectively. The data is mostly geospatial as it contains locations of marine places in the world.
The main purpose of Marine Regions is to serve as the geographical backbone of biodiversity databases such as WoRMS or EurOBIS, hence the main user will be scientists that want to geo-reference their research. But it can also be useful for industry, policy and law makers.
There is already a package to read Marine Regions. The need for this second package is explained in one of the package's articles: Why mregions2? . It was also discussed beforehand that a new package was the best approach: https://github.com/ropensci/mregions/pull/62 (Mostly to not break compatibility)
In general I think this does not apply. The only data being collected from users is the user-agent in all HTTP requests. This gets the environment variable
HTTPUserAgent
viagetOption()
in mr_user_agentOn the other hand, Marine Regions has sensitive data products such as the maritime boundaries. It is stated clearly on our disclaimerΒ that the data has no legal value whatsoever.
pkgcheck
items which your package is unable to pass.Technical checks
Confirm each of the following by checking the box.
This package:
Publication options
[x] Do you intend for this package to go 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
- [ ] 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