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

mregions2: Access Data from Marineregions.org: The Marine Regions Gazetteer and the Marine Regions Data Products #590

Open
13 of 29 tasks
salvafern opened this issue Apr 26, 2023 · 56 comments

Comments

@salvafern
Copy link

salvafern commented Apr 26, 2023

Submitting Author Name: Salvador Fernández Bejarano
Submitting Author Github Handle: @salvafern
Other Package Authors Github handles: (comma separated, delete if none) @lottepohl
Repository: https://github.com/lifewatch/mregions2
Version submitted:
Submission type: Standard
Editor: @jooolia
Reviewers: @Kattuvan, @sheilasaia

Due date for @Kattuvan: 2023-06-20

Due date for @sheilasaia: 2023-06-21
Archive: TBD
Version accepted: TBD
Language: en


  • Paste the full DESCRIPTION file inside a code block below:
Package: mregions2
Type: Package
Title: Access Data from Marineregions.org: The Marine Regions Gazetteer and the Marine Regions Data Products
Version: 1.0.0
Authors@R: c(
  person(given = "Salvador", family = "Fernandez-Bejarano", role = c("aut", "cre"),
         email = "salvador.fernandez@vliz.be",
         comment = c(ORCID = "0000-0003-0535-7677", github = "salvafern")),
  person(given = "Lotte",family = "Pohl", role = "aut",
         email = "lotte.pohl@imbrsea.eu",
         comment = c(ORCID = "0000-0002-7607-7018", github = "whaleshark99")),
  person("LifeWatch Belgium", role = "fnd", comment = "https://lifewatch.be")
  )
Maintainer: Salvador Fernandez-Bejarano <salvador.fernandez@vliz.be>
Description: Explore and retrieve marine geo-spatial data from the Marine Regions Gazetteer and the Marine Regions Data Products, including the Maritime Boundaries.
License: MIT + file LICENSE
Encoding: UTF-8
LazyData: true
Config/testthat/edition: 3
Imports: 
    checkmate,
    glue,
    httr2,
    magrittr,
    sf,
    utils,
    rdflib,
    ISOcodes,
    memoise,
    cli,
    dplyr,
    xml2,
    wrapr,
    methods,
    curl,
    digest
RoxygenNote: 7.2.0
URL: https://github.com/lifewatch/mregions2,
    https://lifewatch.github.io/mregions2/
BugReports: https://github.com/lifewatch/mregions2/issues
Suggests: 
    ows4R (>= 0.3),
    httptest2,
    jsonlite,
    knitr,
    leaflet,
    leaflet.extras2,
    mapview,
    mregions,
    rmarkdown,
    testthat,
    wk,
    purrr,
    withr
VignetteBuilder: knitr
Roxygen: list(markdown = TRUE)
Depends: 
    R (>= 2.10)

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.):
    • data retrieval
    • data extraction
    • data munging
    • data deposition
    • data validation and testing
    • workflow automation
    • version control
    • citation management and bibliometrics
    • scientific software wrappers
    • field and lab reproducibility tools
    • database software bindings
    • geospatial data
    • text analysis
  • Explain how and why the package falls under these categories (briefly, 1-2 sentences):

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.

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

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: ropensci/mregions#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 via getOption() in mr_user_agent

On 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.

  • If you made a pre-submission inquiry, please paste the link to the corresponding issue, forum post, or other discussion, or @tag the editor you contacted.
  • Explain reasons for any pkgcheck items which your package is unable to pass.

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?

  • 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)
  • (Scope: Do consider MEE's Aims and Scope for your manuscript. We make no guarantee that your manuscript will be within MEE scope.)
  • (Although not required, we strongly recommend having a full manuscript prepared when you submit here.)
  • (Please do not submit your package separately to Methods in Ecology and Evolution)

Code of conduct

@ropensci-review-bot
Copy link
Collaborator

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

@ropensci-review-bot
Copy link
Collaborator

🚀

Editor check started

👋

@ropensci-review-bot
Copy link
Collaborator

Checks for mregions2 (v1.0.0)

git hash: 9ed0de6d

  • ✔️ 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 continuous integration checks.
  • ✔️ Package coverage is 91.5%.
  • ✔️ R CMD check found no errors.
  • ✔️ R CMD check found no warnings.

Package License: MIT + file LICENSE


1. Package Dependencies

Details 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.

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(<path/to/repo>)', and examining the 'external_calls' table.

magrittr

%>% (200)

base

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)

mregions2

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)

httr2

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)

checkmate

assert_character (15), assert_integerish (15), assert_int (4), assert_logical (4), assert_numeric (3), assert_data_frame (2), assert_double (2)

stats

offset (18), filter (5), df (1)

methods

coerce (18), is (1)

glue

glue (18)

sf

st_read (3), st_as_sfc (2), st_bbox (2), st_crs (2), st_point (2), st_sfc (2), st_coordinates (1)

leaflet

tileOptions (3), leafletCRS (2), leafletOptions (2), addTiles (1), leaflet (1), WMSTileOptions (1)

cli

cli_abort (7)

memoise

memoise (5)

dplyr

right_join (2), bind_rows (1)

utils

unzip (3)

digest

digest (2)

ISOcodes

ISO_639_2$Alpha_2 (1), ISO_639_2$Name (1)

rdflib

rdf_serialize (1)

xml2

xml_attr (1)

NOTE: Some imported packages appear to have no associated function calls; please ensure with author that these 'Imports' are listed appropriately.


2. Statistical Properties

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

Details of statistical properties (click to open)

The package has:

  • code in R (100% in 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

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

2a. Network visualisation

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


3. goodpractice and other checks

Details of goodpractice checks (click to open)

3a. Continuous Integration Badges

R-CMD-check.yaml
pkgcheck

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

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

Package coverage: 91.54

Cyclocomplexity with cyclocomp

The following functions have cyclocomplexity >= 15:

function cyclocomplexity
gaz_rest_records_by_name 19
gaz_add_geometry 15

Static code analyses with lintr

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 Versions

package version
pkgstats 0.1.3.4
pkgcheck 0.1.1.23


Editor-in-Chief Instructions:

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

@maurolepore
Copy link
Member

Thanks @salvafern for this submission and for pointing to ropensci/mregions#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?

@salvafern
Copy link
Author

Hi @maurolepore, thanks for checking out! 🚀

I am actually the current maintainer of mregions (ropensci/mregions@e5fb45b). Before me were @LennertSchepers and @sckott (ropensci/mregions@dd7b6e4).

Since we had this public conversation on ropensci/mregions#62, and also got the idea of a new package from @maelle (see ropensci/mregions#62 (comment)), 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 😃

@sckott
Copy link
Contributor

sckott commented Apr 27, 2023

No objection from me

@LennertSchepers
Copy link

No objection from me, @salvafern is in our team so we discussed the whole process in detail.

@LennertSchepers
Copy link

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)

@maurolepore
Copy link
Member

Thanks @salvafern and everyone else for your support.
I'll stat searching for a handling editor.

@maelle
Copy link
Member

maelle commented May 2, 2023

@ropensci-review-bot assign @jooolia as editor

@ropensci-review-bot
Copy link
Collaborator

Assigned! @jooolia is now the editor

@jooolia
Copy link
Member

jooolia commented May 5, 2023

Editor checks:

  • 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,

    • Is the case for the package well made?
    • Is the reference index page clear (grouped by topic if necessary)?
    • Are vignettes readable, sufficiently detailed and not just perfunctory?
  • Fit: The package meets criteria for fit and overlap.

  • Installation instructions: Are installation instructions clear enough for human users?

  • Tests: If the package has some interactivity / HTTP / plot production etc. are the tests using state-of-the-art tooling?

    • Yes httptest2
  • Contributing information: Is the documentation for contribution clear enough e.g. tokens for tests, playgrounds?

  • License: The package has a CRAN or OSI accepted license.

  • Project management: Are the issue and PR trackers in a good shape, e.g. are there outstanding bugs, is it clear when feature requests are meant to be tackled?
    Github action checks are kept open as issues, one for each branch.


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


@jooolia
Copy link
Member

jooolia commented May 5, 2023

@ropensci-review-bot seeking reviewers

@ropensci-review-bot
Copy link
Collaborator

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

@jooolia
Copy link
Member

jooolia commented May 26, 2023

Hello, I am still actively looking for reviewers. Thanks, Julia

@Kattuvan
Copy link

@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

@Kattuvan
Copy link

@jooolia I found the guidelines https://devguide.ropensci.org/reviewerguide.html
I am sorry for my earlier mail

@jooolia
Copy link
Member

jooolia commented May 30, 2023

@ropensci-review-bot assign @Kattuvan as reviewer

@ropensci-review-bot
Copy link
Collaborator

@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.

@ropensci-review-bot
Copy link
Collaborator

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

@jooolia
Copy link
Member

jooolia commented May 30, 2023

Thanks @Kattuvan for agreeing to review! Glad that you found the guide. :)

@jooolia
Copy link
Member

jooolia commented May 31, 2023

@ropensci-review-bot assign @sheilasaia as reviewer

@ropensci-review-bot
Copy link
Collaborator

@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.

@jooolia
Copy link
Member

jooolia commented Jun 30, 2023

Hi @salvafern,
Thanks for letting us know. That is fine, talk to you when you are back.
Cheers, Julia

@jooolia
Copy link
Member

jooolia commented Sep 13, 2023

Hi @salvafern,
Hope that you are well. Do you have any updates on your progress?
Thanks, Julia

@salvafern
Copy link
Author

Hi @jooolia

Yes I am currently addressing the reviews (see lifewatch/mregions2#1 (comment) , 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.

@salvafern
Copy link
Author

salvafern commented Sep 19, 2023

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 (lifewatch/mregions2#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: lifewatch/mregions2@de65fbc

@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: lifewatch/mregions2@dcbd99e 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: lifewatch/mregions2@dcbd99e

* ☒ **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: lifewatch/mregions2@de65fbc lifewatch/mregions2@78df1ad lifewatch/mregions2@4c1e224

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: lifewatch/mregions2@40cdf99

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: lifewatch/mregions2@6a7b244 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: lifewatch/mregions2@18696d1

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: lifewatch/mregions2#20
For now I did: lifewatch/mregions2@ca09f8a

  • Added a data frame to the pkg with the definitions mrp_ontology
  • Added an article explaining more background and pretty printing this data frame

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: lifewatch/mregions2@e0a571c

@sheilasaia
Copy link

sheilasaia commented Oct 11, 2023

@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.
screen shot

@salvafern
Copy link
Author

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 (lifewatch/mregions2@a86a1d4). Let me know if it is better or you would still remove it from the docs.

@sheilasaia
Copy link

Looks good to me, @salvafern! Thank you.

@jooolia
Copy link
Member

jooolia commented Nov 25, 2023

Hello @salvafern, sorry for the delay in my handling. I missed these updates. :(
Thank you for all of the work!

@jooolia
Copy link
Member

jooolia commented Nov 25, 2023

Hi @Kattuvan,
Do you feel that the changes and updates have addressed your concerns?
Thanks, Julia

@salvafern
Copy link
Author

Hi @jooolia, did you receive any message from @Kattuvan ? Any chance to move this submission forward?
Best, Salva

@Kattuvan
Copy link

Kattuvan commented Feb 24, 2024 via email

@salvafern
Copy link
Author

Hi @jooolia , please let me know if there is anything else I can do on my side to move this submission forward.
Best, Salva

@jooolia
Copy link
Member

jooolia commented Mar 4, 2024 via email

@jooolia
Copy link
Member

jooolia commented Mar 10, 2024

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!

@salvafern
Copy link
Author

Hi @jooolia, apologies for the delay. The logs are fresh now: https://github.com/lifewatch/mregions2/actions/runs/8615096758/job/23609894330

I think the same issue continues: See lifewatch/mregions2#21 and ropensci/rdflib#47

Any ideas are highly appreciated!

@jooolia
Copy link
Member

jooolia commented Apr 28, 2024

Thanks @salvafern for the reruns. I will have a look and get back to you (and it seems that rdflib has commented out the ubuntu tests for now). thanks, Julia

@jooolia
Copy link
Member

jooolia commented May 5, 2024

Hi @salvafern , I have forked mregions2 and will look into the github actions tomorrow. thanks for your patience.

@salvafern
Copy link
Author

That's great, thank you!

@jooolia
Copy link
Member

jooolia commented May 7, 2024

Hi @salvafern, I have gotten R-CMD-check to run without errors, although it is a bit of a workaround at the moment. I will look into the yaml files tonight and update you then (or if all goes well submit a pull request) with what has worked and what we could imagine doing.
Cheers, Julia

@jooolia
Copy link
Member

jooolia commented May 20, 2024

Hi @salvafern I have opened a PR in the repo to your dev branch. Let me know what you think. I had to remove pkgcheck, but all three others I was able to get to work and pass.
Cheers, Julia

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

9 participants