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

mapmetadata #674

Open
13 of 29 tasks
RayStick opened this issue Nov 26, 2024 · 75 comments
Open
13 of 29 tasks

mapmetadata #674

RayStick opened this issue Nov 26, 2024 · 75 comments

Comments

@RayStick
Copy link

RayStick commented Nov 26, 2024

Submitting Author Name: Rachael Stickland
Submitting Author Github Handle: @RayStick
Other Package Authors Github handles: (comma separated, delete if none) @BatoolMM, @Rainiefantasy
Repository: https://github.com/aim-rsf/mapmetadata
Version submitted:
Submission type: Standard
Editor: @maelle
Reviewers: @ymansiaux, @Lextuga007

Archive: TBD
Version accepted: TBD
Language: en


  • Paste the full DESCRIPTION file inside a code block below:
Type: Package
Package: mapmetadata
Title: Map health metadata onto predefined research domains
Version: 3.0.0
Authors@R: c(
    person("Rachael", "Stickland", , "rstickland@turing.ac.uk", role = c("aut", "cre"),
           comment = c(ORCID = "0000-0003-3398-4272")),
    person("Batool", "Almarzouq", role = "ctb",
           comment = c(ORCID = "0000-0002-3905-2751")),
    person("Mahwish", "Mohammad", role = "ctb",
           comment = c(ORCID = "0009-0004-5295-0726")),
    person("Daniel", "Delbarre", role = "ctb",
           comment = c(ORCID = "0000-0003-4633-4252")),
    person("Nida", "Ziauddeen", role = "ctb",
           comment = c(ORCID = "0000-0002-8964-5029"))
  )
Description: Prior to gaining full access to health datasets, explore
    publicly available metadata and map metadata onto predefined research
    domains.  This package uses structural metadata files downloaded from
    the Health Data Research Gateway (https://healthdatagateway.org/en).
    In theory, any metadata file with the same structure as the files
    downloaded from this gateway can be used with this package, but the
    package has been developed and tested on metadata files from this
    gateway only.
License: GPL (>= 3)
URL: https://aim-rsf.github.io/mapmetadata/
BugReports: https://github.com/aim-rsf/mapmetadata/issues
Depends: 
    R (>= 2.10)
Imports: 
    cli,
    dplyr,
    ggplot2,
    gridExtra,
    htmlwidgets,
    plotly,
    tidyr
Suggests: 
    devtools,
    knitr,
    mockery,
    rmarkdown,
    testthat (>= 3.0.0),
    withr
VignetteBuilder: 
    knitr
Config/testthat/edition: 3
Encoding: UTF-8
LazyData: true
RoxygenNote: 7.3.2

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 is related to data access, as interacting with health metadata can help a researcher/research group decide what datasets to access for their research, and be more informed when writing their data access request. It involves data validation checks as it checks for completeness of metadata, and visualizes this.

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

Any users of health metadata, specifically for research projects that are using large population datasets, with many latent variables (research domains/concepts) and they need to investigate which variables in the datasets map onto their research domains of interest.

Not that I am aware of

Not Applicable

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

Not Applicable

  • 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

👋

@RayStick RayStick changed the title [WIP] Rachael Stickland [WIP] browseMetadata Nov 26, 2024
@ropensci-review-bot
Copy link
Collaborator

Checks for browseMetadata (v2.0.1)

git hash: 3a779939

  • ✔️ Package name is available
  • ✔️ has a 'codemeta.json' file.
  • ✔️ has a 'contributing' file.
  • ✔️ uses 'roxygen2'.
  • ✔️ 'DESCRIPTION' has a URL field.
  • ✖️ 'DESCRIPTION' does not have a BugReports field.
  • ✔️ Package has at least one HTML vignette
  • ✖️ These functions do not have examples: [concensus_on_mismatch, copy_previous, count_empty_desc, end_plot, join_outputs, json_table_to_df, load_data, map_metadata_compare, map_metadata_convert, ref_plot, user_categorisation_loop, user_categorisation, user_prompt_list, user_prompt, valid_comparison].
  • ✖️ Package has no continuous integration checks.
  • ✖️ Package coverage is 53.3% (should be at least 75%).
  • ✔️ R CMD check found no errors.
  • ✔️ R CMD check found no warnings.
  • 👀 Function names are duplicated in other packages

Important: All failing checks above must be addressed prior to proceeding

(Checks marked with 👀 may be optionally addressed.)

Package License: GPL (>= 3)


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 206
internal browseMetadata 33
internal stats 15
internal graphics 13
internal utils 8
internal tools 1
imports cli 30
imports dplyr 9
imports tidyr 3
imports gridExtra 2
imports htmlwidgets 2
imports plotly 2
imports jsonlite 2
imports ggplot2 1
suggests knitr NA
suggests rmarkdown NA
suggests devtools NA
suggests testthat NA
suggests mockery 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.

base

list (57), paste0 (15), data.frame (13), c (11), for (10), return (9), character (8), nrow (8), length (6), get (5), file (3), format (3), integer (3), numeric (3), row (3), Sys.time (3), all (2), apply (2), as.list (2), list.files (2), max (2), min (2), paste (2), rbind (2), readline (2), strsplit (2), subset (2), suppressWarnings (2), t (2), unique (2), unlist (2), any (1), as.integer (1), as.matrix (1), cbind (1), do.call (1), getwd (1), gsub (1), is.na (1), lapply (1), matrix (1), nchar (1), scan (1), setdiff (1), system.file (1), unname (1), which (1)

browseMetadata

json_table_to_df (4), user_categorisation (4), ref_plot (3), user_prompt_list (3), concensus_on_mismatch (2), copy_previous (2), count_empty_desc (2), end_plot (2), join_outputs (2), load_data (2), user_prompt (2), browse_metadata (1), map_metadata (1), map_metadata_compare (1), map_metadata_convert (1), user_categorisation_loop (1)

cli

cli_alert_info (17), cli_alert_danger (4), cli_alert_success (4), cli_h1 (3), cli_alert_warning (2)

stats

family (8), line (6), df (1)

graphics

title (7), legend (3), text (3)

dplyr

n (7), join_by (1), left_join (1)

utils

read.csv (7), data (1)

tidyr

complete (3)

gridExtra

grid.arrange (1), tableGrob (1)

htmlwidgets

saveWidget (2)

jsonlite

fromJSON (2)

plotly

plot_ly (2)

ggplot2

ggsave (1)

tools

file_path_sans_ext (1)


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 11 files) and
  • 1 authors
  • 2 vignettes
  • 5 internal data files
  • 8 imported packages
  • 17 exported functions (median 23 lines of code)
  • 17 non-exported functions in R (median 24 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 11 60.1
files_vignettes 2 81.7
files_tests 15 92.3
loc_R 725 56.6
loc_vignettes 190 45.1
loc_tests 410 67.1
num_vignettes 2 85.5
data_size_total 24603 74.8
data_size_median 245 55.6
n_fns_r 34 43.4
n_fns_r_exported 17 62.1
n_fns_r_not_exported 17 35.4
n_fns_per_file_r 2 34.1
num_params_per_fn 3 29.3
loc_per_fn_r 24 68.0
loc_per_fn_r_exp 23 52.4
loc_per_fn_r_not_exp 24 70.5
rel_whitespace_R 19 58.9
rel_whitespace_vignettes 43 54.0
rel_whitespace_tests 25 70.1
doclines_per_fn_exp 17 9.3
doclines_per_fn_not_exp 0 0.0 TRUE
fn_call_network_size 31 54.8

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)


3b. goodpractice results

R CMD check with rcmdcheck

R CMD check generated the following note:

  1. checking R code for possible problems ... NOTE
    browse_metadata: no visible binding for global variable ‘Empty’
    copy_previous: no visible binding for global variable ‘data_element’
    count_empty_desc: no visible binding for global variable ‘empty’
    end_plot: no visible binding for global variable ‘domain_code’
    join_outputs: no visible binding for global variable ‘data_element’
    map_metadata: no visible binding for global variable ‘note’
    Undefined global functions or variables:
    data_element domain_code empty Empty note

R CMD check generated the following check_fails:

  1. description_bugreports
  2. no_import_package_as_a_whole
  3. rcmdcheck_undefined_globals

Test coverage with covr

Package coverage: 53.31

The following files are not completely covered by tests:

file coverage
R/map_metadata_compare.R 0%
R/map_metadata_convert.R 0%
R/map_metadata.R 0%

Cyclocomplexity with cyclocomp

The following function have cyclocomplexity >= 15:

function cyclocomplexity
map_metadata 18

Static code analyses with lintr

lintr found no issues with this package!


4. Other Checks

Details of other checks (click to open)

✖️ The following function name is duplicated in other packages:

    • browse_metadata from OECD


Package Versions

package version
pkgstats 0.2.0.48
pkgcheck 0.1.2.77


Editor-in-Chief Instructions:

Processing may not proceed until the items marked with ✖️ have been resolved.

@RayStick
Copy link
Author

Closing issue for now, as I misread some recommendations for a package to be ready. Will address these properly (the pkgcheck results) and then re-submit. Thank you!

@mpadge
Copy link
Member

mpadge commented Nov 27, 2024

@RayStick No worries. When you're ready, please just open this issue again (and not a new issue), call @ropensci-review-bot check package and things will move on from there.

@RayStick
Copy link
Author

Okay I will do that, thanks @mpadge!

@RayStick RayStick reopened this Dec 12, 2024
@RayStick
Copy link
Author

@ropensci-review-bot check package

@ropensci-review-bot
Copy link
Collaborator

Thanks, about to send the query.

@ropensci-review-bot
Copy link
Collaborator

🚀

Editor check started

👋

@RayStick RayStick changed the title [WIP] browseMetadata browseMetadata Dec 12, 2024
@ropensci-review-bot
Copy link
Collaborator

Checks for browseMetadata (v2.0.2)

git hash: 57b7191b

  • ✔️ 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 89.2%.
  • ✔️ R CMD check found no errors.
  • ✔️ R CMD check found no warnings.
  • 👀 Function names are duplicated in other packages

(Checks marked with 👀 may be optionally addressed.)

Package License: GPL (>= 3)


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 206
internal browseMetadata 33
internal stats 15
internal graphics 13
internal utils 8
internal tools 1
imports cli 30
imports dplyr 9
imports tidyr 3
imports gridExtra 2
imports htmlwidgets 2
imports plotly 2
imports jsonlite 2
imports ggplot2 1
suggests knitr NA
suggests rmarkdown NA
suggests devtools NA
suggests testthat NA
suggests mockery 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.

base

list (57), paste0 (15), data.frame (13), c (11), for (10), return (9), character (8), nrow (8), length (6), get (5), file (3), format (3), integer (3), numeric (3), row (3), Sys.time (3), all (2), apply (2), as.list (2), list.files (2), max (2), min (2), paste (2), rbind (2), readline (2), strsplit (2), subset (2), suppressWarnings (2), t (2), unique (2), unlist (2), any (1), as.integer (1), as.matrix (1), cbind (1), do.call (1), getwd (1), gsub (1), is.na (1), lapply (1), matrix (1), nchar (1), scan (1), setdiff (1), system.file (1), unname (1), which (1)

browseMetadata

json_table_to_df (4), user_categorisation (4), ref_plot (3), user_prompt_list (3), concensus_on_mismatch (2), copy_previous (2), count_empty_desc (2), end_plot (2), join_outputs (2), load_data (2), user_prompt (2), browse_metadata (1), map_metadata (1), map_metadata_compare (1), map_metadata_convert (1), user_categorisation_loop (1)

cli

cli_alert_info (17), cli_alert_danger (4), cli_alert_success (4), cli_h1 (3), cli_alert_warning (2)

stats

family (8), line (6), df (1)

graphics

title (7), legend (3), text (3)

dplyr

n (7), join_by (1), left_join (1)

utils

read.csv (7), data (1)

tidyr

complete (3)

gridExtra

grid.arrange (1), tableGrob (1)

htmlwidgets

saveWidget (2)

jsonlite

fromJSON (2)

plotly

plot_ly (2)

ggplot2

ggsave (1)

tools

file_path_sans_ext (1)


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 11 files) and
  • 1 authors
  • 2 vignettes
  • 5 internal data files
  • 8 imported packages
  • 17 exported functions (median 23 lines of code)
  • 17 non-exported functions in R (median 24 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 11 60.2
files_vignettes 2 81.7
files_tests 18 93.9
loc_R 728 56.8
loc_vignettes 191 45.5
loc_tests 519 72.1
num_vignettes 2 85.5
data_size_total 24603 74.8
data_size_median 245 55.6
n_fns_r 34 43.4
n_fns_r_exported 17 62.2
n_fns_r_not_exported 17 35.5
n_fns_per_file_r 2 34.1
num_params_per_fn 3 29.4
loc_per_fn_r 24 68.0
loc_per_fn_r_exp 23 52.5
loc_per_fn_r_not_exp 24 70.6
rel_whitespace_R 19 58.8
rel_whitespace_vignettes 43 54.4
rel_whitespace_tests 25 74.9
doclines_per_fn_exp 18 11.1
doclines_per_fn_not_exp 0 0.0 TRUE
fn_call_network_size 31 54.8

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 R-CMD-check
graph codecov

GitHub Workflow Results

id name conclusion sha run_number date
12298710868 Auto Author Assign success 57b719 72 2024-12-12
12293411548 auto-label success be2574 226 2024-12-12
12293835316 pages build and deployment success f8c518 121 2024-12-12
12293615596 pkgcheck success 57b719 13 2024-12-12
12293809892 pkgdown success 57b719 401 2024-12-12
12293615592 R-CMD-check.yaml success 57b719 73 2024-12-12
12293615597 test-coverage.yaml success 57b719 73 2024-12-12

3b. goodpractice results

R CMD check with rcmdcheck

R CMD check generated the following notes:

  1. checking for portable file names ... NOTE
    Found the following non-portable file paths:
    browseMetadata/inst/outputs/L-OUTPUT_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-11-27-14-19-55.csv
    browseMetadata/inst/outputs/LOG_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-11-27-14-19-55.csv
    browseMetadata/inst/outputs/LOG_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-11-27-14-23-52.csv
    browseMetadata/inst/outputs/OUTPUT_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-11-27-14-19-55.csv
    browseMetadata/inst/outputs/OUTPUT_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-11-27-14-23-52.csv
    browseMetadata/inst/outputs/PLOT_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-11-27-14-19-55.png
    browseMetadata/inst/outputs/PLOT_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-11-27-14-23-52.png

Tarballs are only required to store paths of up to 100 bytes and cannot
store those of more than 256 bytes, with restrictions including to 100
bytes for the final component.
See section ‘Package structure’ in the ‘Writing R Extensions’ manual.
2. checking R code for possible problems ... NOTE
browse_metadata: no visible binding for global variable ‘Empty’
copy_previous: no visible binding for global variable ‘data_element’
count_empty_desc: no visible binding for global variable ‘empty’
end_plot: no visible binding for global variable ‘domain_code’
join_outputs: no visible binding for global variable ‘data_element’
map_metadata: no visible binding for global variable ‘note’
Undefined global functions or variables:
data_element domain_code empty Empty note

R CMD check generated the following check_fails:

  1. no_import_package_as_a_whole
  2. rcmdcheck_undefined_globals

Test coverage with covr

Package coverage: 89.15

Cyclocomplexity with cyclocomp

The following function have cyclocomplexity >= 15:

function cyclocomplexity
map_metadata 18

Static code analyses with lintr

lintr found no issues with this package!


4. Other Checks

Details of other checks (click to open)

✖️ The following function name is duplicated in other packages:

    • browse_metadata from OECD


Package Versions

package version
pkgstats 0.2.0.48
pkgcheck 0.1.2.77


Editor-in-Chief Instructions:

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

@BatoolMM
Copy link

Thank you soooo much @RayStick!!!

@emilyriederer
Copy link

Thanks for the submission, @RayStick ! This looks like a great candidate for rOpenSci. I'll begin the search for a handling editor

@emilyriederer
Copy link

@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/674_status.svg)](https://github.com/ropensci/software-review/issues/674)

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

@maelle
Copy link
Member

maelle commented Dec 16, 2024

@ropensci-review-bot assign @maelle as editor

@ropensci-review-bot
Copy link
Collaborator

Assigned! @maelle is now the editor

@maelle
Copy link
Member

maelle commented Dec 16, 2024

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

Editor comments

Thank you for your submission! Below are some comments before I start looking for reviewers. Please feel free to ask me any question.


@maelle
Copy link
Member

maelle commented Dec 16, 2024

I removed the seeking-reviewers label because I'll look for reviewers after your response @RayStick 🙂

@RayStick
Copy link
Author

RayStick commented Dec 16, 2024

Thanks for these comments @maelle
This morning, I am going to add an rOpenSci review badge to my README, and add a NEWS file
Just to check - is the idea that I respond and implement your suggested changes above before it goes to review?

@maelle
Copy link
Member

maelle commented Dec 16, 2024

Ideally yes, either implementing or rejecting them (the first item on documenting scope is the most important one IMO) so the reviewers don't have to comment on the same components. Does it make sense? (it's a busy season so I expect things to take a while longer, no pressure)

@RayStick
Copy link
Author

Thanks! Most seem pretty do-able to me, and I should have a bit of time today to go through them

@ropensci-review-bot
Copy link
Collaborator

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

@maelle
Copy link
Member

maelle commented Jan 9, 2025

@ropensci-review-bot add @Lextuga007 to reviewers

@ropensci-review-bot
Copy link
Collaborator

@Lextuga007 added to the reviewers list. Review due date is 2025-01-30. Thanks @Lextuga007 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

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

@maelle
Copy link
Member

maelle commented Jan 9, 2025

@ymansiaux @Lextuga007 thanks so much to you both for accepting to review this package 🙏

@ymansiaux
Copy link

Hi @maelle and @RayStick , please find below my review.

Package 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
  • Examples: (that run successfully locally) for all exported functions
  • 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 have been confirmed.
  • Performance: Any performance claims of the software have 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.

Please see below my review comments. I think that some modifications should be made, for example in the README or function examples.

Estimated hours spent reviewing: 7

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

Review Comments

Thank you very much for sharing this work and for your high-quality development.

I particularly appreciate the attention you've paid to documentation and unit testing. I’ve personally learned a lot, especially about mocking, which I hadn’t explored much until now.

Below, please find my detailed comments and suggestions:

Function and argument naming

To align with the naming of the other exported functions, map_compare() and map_convert(), I would suggest renaming metadata_map() to map_metadata(). While I understand you may have concerns about potential confusion with the package name, I believe this change would harmonize the naming scheme, enhance clarity, and facilitate autocomplete for users.

For the map_convert() function, I recommend renaming the argument output_csv to csv_to_convert to make its purpose clearer. Additionally, introducing a new argument, csv_to_convert_dir, could address an issue I’ve identified in the provided function example (see the Examples section below). If you implement this change, the output_dir argument could represent the directory where the generated CSV file is stored (defaulting to csv_to_convert_dir if not specified).

Input checking

I suggest adding explicit input validation to your exported functions. This would enhance the user experience by providing clear and informative error messages or warnings.

  • For map_compare(), consider verifying that session_dir exists and that the CSV files specified in the user input are present.
  • For map_convert(), you might want to check that both output_csv and output_dir exist.
  • For metadata_map() and the other functions, validating file existence, expected dimensions, and column names (if any) could further improve robustness.

README

The demo example in the README feels somewhat extensive and might benefit from being simplified to improve accessibility. Currently, interacting with up to 20 variables requires substantial effort, which could deter users from running the full demo.

For sections involving custom inputs (metadata input, domain list input, and custom lookup), I recommend moving these to the vignette and referencing it in the README. For example:

"If you wish to use custom metadata, domain lists, or lookup tables, please refer to vignette XXX, section YYY."

Additionally, examples in the README should be copy-pasteable and work seamlessly without requiring additional setup. For instance, the following code snippet requires downloading a file.

new_csv_file <- "path/your_new_csv.csv"
demo_domains_file <- system.file("inputs/domain_list_demo.csv", package = "mapmetadata")

metadata_map(csv_file = new_csv_file, domain_file = demo_domains_file)

This is why I would present only the demo example in the README.

Providing examples for custom domain list inputs and custom lookup tables would also be beneficial. For these examples, linking to demo files in inst/inputs could clarify the expected file formats. I guess an explanation about the location of the files used by default and the way to access them programmatically with system.file() would be useful to your users (If this function is probably familiar to R packages developpers, it might not be straightforward for package “users” only).

Documentation

For non-exported functions, you might want to prevent .Rd files from being generated in the man/ folder. Adding the #' @noRd tag to these functions' documentation can achieve this. For more details, see [the rOpenSci dev guide](https://devguide.ropensci.org/pkg_building.html#roxygen-2-use).

Testing

In the tests/testthat.R file, it seems unnecessary to load the {mockery} and {withr} packages explicitly, as you are already using explicit namespacing.

A note regarding {mockery}: its README states that the package is superseded and recommends using testthat::local_mocked_bindings() instead. More details can be found [here](https://github.com/r-lib/mockery).

Examples

Some examples are enclosed in dontrun tags, which may pose an issue if you plan to submit the package to CRAN.

  • For map_compare(), consider using a temporary directory to avoid creating files in the user’s workspace or package’s installation directory:
# Locate file paths for the example files in the package
demo_session_dir <- system.file("outputs", package = "mapmetadata")
demo_session1_base <- "360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-11-55"
demo_session2_base <- "360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-17-45"
demo_metadata_file <- system.file("inputs", "360_NationalCommunityChildHealthDatabase(NCCHD)_Structural_Metadata.csv", package = "mapmetadata")
demo_domain_file <- system.file("inputs", "domain_list_demo.csv", package = "mapmetadata")
output_dir <- tempdir()
message("Output directory: ", output_dir)

# Run the function - requires user interaction
map_compare(
  session_dir = demo_session_dir,
  session1_base = demo_session1_base,
  session2_base = demo_session2_base,
  metadata_file = demo_metadata_file,
  domain_file = demo_domain_file,
  output_dir = output_dir
)

Returning the path of the output file and including a browseURL() call to open the file in the example could enhance usability. This would allow us to delete the temp directory once the generated file has been opened.

The example for map_convert() writes to the package’s installation directory, which is problematic. Modifying this to use temporary directories would improve clarity and compliance. If you implement my earlier suggestion to add a csv_to_convert_dir argument, this example could be significantly simplified. Moreover, I think a message telling the user the path of the output could be nice, like you did with map_compare(). I think this could be nice that this function returns the path of the output file so we could open it in the example with a browseURL() call.

For metadata_map(), since it requires user interaction, you should enclose it in an if(interactive()) block:

#' @examples
# Demo run requires no function inputs but requires user interaction
if(interactive()) {
	metadata_map()
}

file path construction

In your file-handling functions, you are constructing paths using paste() or paste0(). To ensure platform independence, I recommend replacing these with file.path(). For example, in map_compare():

You could replace

  csv_1a <- read.csv(paste0(session_dir, "/", "MAPPING_LOG_", session1_base, ".csv"))

with

  csv_1a <- read.csv(file.path(session_dir, paste0("MAPPING_LOG_", session1_base, ".csv")))

This ensures compatibility across operating systems.

devtools::check()

Here is my session info

sessionInfo()
R version 4.4.1 (2024-06-14 ucrt)
Platform: x86_64-w64-mingw32/x64
Running under: Windows 11 x64 (build 22631)

Matrix products: default

locale:
[1] LC_COLLATE=French_France.utf8  LC_CTYPE=French_France.utf8   
[3] LC_MONETARY=French_France.utf8 LC_NUMERIC=C                  
[5] LC_TIME=French_France.utf8    

time zone: Europe/Paris
tzcode source: internal

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

other attached packages:
[1] mapmetadata_3.0.0 testthat_3.2.2   

loaded via a namespace (and not attached):
 [1] gtable_0.3.6      jsonlite_1.8.9    dplyr_1.1.4      
 [4] compiler_4.4.1    brio_1.1.5        tidyselect_1.2.1 
 [7] gridExtra_2.3     tidyr_1.3.1       scales_1.3.0     
[10] yaml_2.3.10       fastmap_1.2.0     ggplot2_3.5.1    
[13] R6_2.5.1          generics_0.1.3    knitr_1.49       
[16] htmlwidgets_1.6.4 tibble_3.2.1      desc_1.4.3       
[19] munsell_0.5.1     rprojroot_2.0.4   pillar_1.10.1    
[22] rlang_1.1.4.9000  xfun_0.50         fs_1.6.5         
[25] lazyeval_0.2.2    pkgload_1.4.0     viridisLite_0.4.2
[28] plotly_4.10.4     cli_3.6.3.9000    withr_3.0.2      
[31] magrittr_2.0.3    crosstalk_1.2.1   digest_0.6.37    
[34] grid_4.4.1        rstudioapi_0.17.1 lifecycle_1.0.4  
[37] vctrs_0.6.5       evaluate_1.0.3    glue_1.8.0       
[40] data.table_1.16.4 farver_2.1.2      pkgbuild_1.4.5   
[43] colorspace_2.1-1  rmarkdown_2.29    purrr_1.0.2      
[46] httr_1.4.7        tools_4.4.1       pkgconfig_2.0.3  
[49] htmltools_0.5.8.1

Below is the result of devtools::check() on my computer :

devtools::check(document = FALSE)

══ Building ═════════════════════════════════════════════════════════════════════════════════════════════════════════════════
Setting env vars:CFLAGS    : -Wall -pedantic -fdiagnostics-color=alwaysCXXFLAGS  : -Wall -pedantic -fdiagnostics-color=alwaysCXX11FLAGS: -Wall -pedantic -fdiagnostics-color=alwaysCXX14FLAGS: -Wall -pedantic -fdiagnostics-color=alwaysCXX17FLAGS: -Wall -pedantic -fdiagnostics-color=alwaysCXX20FLAGS: -Wall -pedantic -fdiagnostics-color=always
── R CMD build ──────────────────────────────────────────────────────────────────────────────────────────────────────────────
✔  checking for file 'C:\Users\yohann\Documents\code\reviewropensci\mapmetadata/DESCRIPTION' (454ms)
─  preparing 'mapmetadata': (1.7s)
✔  checking DESCRIPTION meta-information ...installing the package to build vignettescreating vignettes (7.3s)
─  checking for LF line-endings in source and make files and shell scripts (891ms)
─  checking for empty or unneeded directoriesbuilding 'mapmetadata_3.0.0.tar.gz'
   Avis dans utils::tar(filepath, pkgname, compression = compression, compression_level = 9L,  :
     storing paths of more than 100 bytes is not portable:
     'mapmetadata/inst/outputs/BAR_360_NationalCommunityChildHealthDatabase(NCCHD)_2024-12-19-14-11-55.html'
   Avis dans utils::tar(filepath, pkgname, compression = compression, compression_level = 9L,  :
     storing paths of more than 100 bytes is not portable:
     'mapmetadata/inst/outputs/CONSENSUS_MAPPING_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-38-45.csv'
   Avis dans utils::tar(filepath, pkgname, compression = compression, compression_level = 9L,  :
     storing paths of more than 100 bytes is not portable:
     'mapmetadata/inst/outputs/L-MAPPING_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-11-55.csv'
   Avis dans utils::tar(filepath, pkgname, compression = compression, compression_level = 9L,  :
     storing paths of more than 100 bytes is not portable:
     'mapmetadata/inst/outputs/MAPPING_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-11-55.csv'
   Avis dans utils::tar(filepath, pkgname, compression = compression, compression_level = 9L,  :
     storing paths of more than 100 bytes is not portable:
     'mapmetadata/inst/outputs/MAPPING_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-17-45.csv'
   Avis dans utils::tar(filepath, pkgname, compression = compression, compression_level = 9L,  :
     storing paths of more than 100 bytes is not portable:
     'mapmetadata/inst/outputs/MAPPING_LOG_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-11-55.csv'
   Avis dans utils::tar(filepath, pkgname, compression = compression, compression_level = 9L,  :
     storing paths of more than 100 bytes is not portable:
     'mapmetadata/inst/outputs/MAPPING_LOG_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-17-45.csv'
   Avis dans utils::tar(filepath, pkgname, compression = compression, compression_level = 9L,  :
     storing paths of more than 100 bytes is not portable:
     'mapmetadata/inst/outputs/MAPPING_PLOT_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-11-55.png'
   
══ Checking ═════════════════════════════════════════════════════════════════════════════════════════════════════════════════
Setting env vars:_R_CHECK_CRAN_INCOMING_REMOTE_               : FALSE_R_CHECK_CRAN_INCOMING_                      : FALSE_R_CHECK_FORCE_SUGGESTS_                     : FALSE_R_CHECK_PACKAGES_USED_IGNORE_UNUSED_IMPORTS_: FALSENOT_CRAN                                     : true
── R CMD check ──────────────────────────────────────────────────────────────────────────────────────────────────────────────
─  using log directory 'C:/Users/yohann/Documents/code/reviewropensci/mapmetadata.Rcheck' (480ms)
─  using R version 4.4.1 (2024-06-14 ucrt)
─  using platform: x86_64-w64-mingw32R was compiled by
       gcc.exe (GCC) 13.2.0
       GNU Fortran (GCC) 13.2.0running under: Windows 11 x64 (build 22631)
─  using session charset: UTF-8using options '--no-manual --as-cran' (858ms)
✔  checking for file 'mapmetadata/DESCRIPTION'checking extension type ... Packagethis is package 'mapmetadata' version '3.0.0'package encoding: UTF-8checking package namespace informationchecking package dependencies (1.2s)
✔  checking if this is a source package ...checking if there is a namespacechecking for executable files (926ms)
✔  checking for hidden files and directories ...
N  checking for portable file names
   Found the following non-portable file paths:
     mapmetadata/inst/outputs/BAR_360_NationalCommunityChildHealthDatabase(NCCHD)_2024-12-19-14-11-55.html
     mapmetadata/inst/outputs/CONSENSUS_MAPPING_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-38-45.csv
     mapmetadata/inst/outputs/L-MAPPING_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-11-55.csv
     mapmetadata/inst/outputs/MAPPING_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-11-55.csv
     mapmetadata/inst/outputs/MAPPING_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-17-45.csv
     mapmetadata/inst/outputs/MAPPING_LOG_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-11-55.csv
     mapmetadata/inst/outputs/MAPPING_LOG_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-17-45.csv
     mapmetadata/inst/outputs/MAPPING_PLOT_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-11-55.png
   
   Tarballs are only required to store paths of up to 100 bytes and cannot
   store those of more than 256 bytes, with restrictions including to 100
   bytes for the final component.
   See section 'Package structure' in the 'Writing R Extensions' manual.checking serialization versionschecking whether package 'mapmetadata' can be installed (9.6s)
N  checking installed package size ... 
     installed size is  5.3Mb
     sub-directories of 1Mb or more:
       outputs   4.4Mbchecking package directory (357ms)
N  checking for future file timestamps (4.2s)
   unable to verify current timechecking 'build' directorychecking DESCRIPTION meta-information (349ms)
✔  checking top-level fileschecking for left-over files ...checking index information ...checking package subdirectories (1s)
✔  checking code files for non-ASCII characters ...checking R files for syntax errors ...checking whether the package can be loaded (994ms)
✔  checking whether the package can be loaded with stated dependencies (706ms)
✔  checking whether the package can be unloaded cleanly (780ms)
✔  checking whether the namespace can be loaded with stated dependencies (788ms)
✔  checking whether the namespace can be unloaded cleanly (1.1s)
✔  checking loading without being on the library search path (1.2s)
✔  checking dependencies in R code (2s)
✔  checking S3 generic/method consistency (1.1s)
✔  checking replacement functions (892ms)
✔  checking foreign function calls (790ms)
N  checking R code for possible problems (6.2s)
   empty_count: no visible binding for global variable 'Section'
   empty_count: no visible binding for global variable
     'Column.description'
   empty_count: no visible binding for global variable 'total_variables'
   empty_count: no visible binding for global variable 'no_count'
   empty_count: no visible binding for global variable 'Empty'
   end_plot: no visible binding for global variable 'domain_code'
   map_compare: no visible binding for global variable 'Section'
   map_compare: no visible binding for global variable 'data_element'
   map_compare: no visible global function definition for 'contains'
   metadata_map: no visible binding for global variable 'Section'
   metadata_map: no visible binding for global variable 'note'
   output_copy: no visible binding for global variable 'data_element'
   Undefined global functions or variables:
     Column.description Empty Section contains data_element domain_code
     no_count note total_variableschecking Rd files (571ms)
✔  checking Rd metadata ... 
N  checking Rd line widths ... 
   Rd file 'map_compare.Rd':
     \examples lines wider than 100 characters:
        demo_metadata_file <- system.file("inputs", "360_NationalCommunityChildHealthDatabase(NCCHD)_Structural_Metadata.csv", package = "mapme ... [TRUNCATED]
   
   Rd file 'map_convert.Rd':
     \examples lines wider than 100 characters:
        demo_output_csv <- "MAPPING_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-17-45.csv"
   
   These lines will be truncated in the PDF manual.
✔  checking Rd cross-references ... 
✔  checking for missing documentation entries (925ms)
✔  checking for code/documentation mismatches (2.4s)
✔  checking Rd \usage sections (1.2s)
✔  checking Rd contents ... 
✔  checking for unstated dependencies in examples ... 
✔  checking contents of 'data' directory
✔  checking data for non-ASCII characters ... 
✔  checking LazyData
✔  checking data for ASCII and uncompressed saves ... 
✔  checking installed files from 'inst/doc' ... 
✔  checking files in 'vignettes' ... 
✔  checking examples (1.5s)
✔  checking for unstated dependencies in 'tests' ... 
─  checking tests ...
✔  Running 'testthat.R' (4.7s)
✔  checking for unstated dependencies in vignettes (563ms)
✔  checking package vignettes ...
✔  checking re-building of vignette outputs (4.9s)
✔  checking for non-standard things in the check directory ...
✔  checking for detritus in the temp directory
   
   See
     'C:/Users/yohann/Documents/code/reviewropensci/mapmetadata.Rcheck/00check.log'
   for details.
   
── R CMD check results ─────────────────────────────────────────────────────────────────────────────── mapmetadata 3.0.0 ────
Duration: 1m 0.1s

❯ checking for portable file names ... NOTE
  Found the following non-portable file paths:
    mapmetadata/inst/outputs/BAR_360_NationalCommunityChildHealthDatabase(NCCHD)_2024-12-19-14-11-55.html
    mapmetadata/inst/outputs/CONSENSUS_MAPPING_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-38-45.csv
    mapmetadata/inst/outputs/L-MAPPING_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-11-55.csv
    mapmetadata/inst/outputs/MAPPING_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-11-55.csv
    mapmetadata/inst/outputs/MAPPING_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-17-45.csv
    mapmetadata/inst/outputs/MAPPING_LOG_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-11-55.csv
    mapmetadata/inst/outputs/MAPPING_LOG_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-17-45.csv
    mapmetadata/inst/outputs/MAPPING_PLOT_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-11-55.png
  
  Tarballs are only required to store paths of up to 100 bytes and cannot
  store those of more than 256 bytes, with restrictions including to 100
  bytes for the final component.
  See section 'Package structure' in the 'Writing R Extensions' manual.

❯ checking installed package size ... NOTE
    installed size is  5.3Mb
    sub-directories of 1Mb or more:
      outputs   4.4Mb

❯ checking for future file timestamps ... NOTE
  unable to verify current time

❯ checking R code for possible problems ... NOTE
  empty_count: no visible binding for global variable 'Section'
  empty_count: no visible binding for global variable
    'Column.description'
  empty_count: no visible binding for global variable 'total_variables'
  empty_count: no visible binding for global variable 'no_count'
  empty_count: no visible binding for global variable 'Empty'
  end_plot: no visible binding for global variable 'domain_code'
  map_compare: no visible binding for global variable 'Section'
  map_compare: no visible binding for global variable 'data_element'
  map_compare: no visible global function definition for 'contains'
  metadata_map: no visible binding for global variable 'Section'
  metadata_map: no visible binding for global variable 'note'
  output_copy: no visible binding for global variable 'data_element'
  Undefined global functions or variables:
    Column.description Empty Section contains data_element domain_code
    no_count note total_variables

❯ checking Rd line widths ... NOTE
  Rd file 'map_compare.Rd':
    \examples lines wider than 100 characters:
       demo_metadata_file <- system.file("inputs", "360_NationalCommunityChildHealthDatabase(NCCHD)_Structural_Metadata.csv", package = "mapme ... [TRUNCATED]
  
  Rd file 'map_convert.Rd':
    \examples lines wider than 100 characters:
       demo_output_csv <- "MAPPING_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-17-45.csv"
  
  These lines will be truncated in the PDF manual.

0 errors| 0 warnings| 5 notesR CMD check succeeded

Here are my notes regarding the five devtools::check() outputs:

  • Portable file names: Removing parentheses from file names should resolve this issue.
  • Package size: Large files in inst/outputs, such as BAR_360_NationalCommunityChildHealthDatabase.html, could be removed if not essential to the package.
  • Future file timestamps: Likely specific to my computer and unrelated to your code.
  • Global variable bindings: Create an R/globals.R file and use utils::globalVariables(). For guidance, see [this forum post](https://forum.posit.co/t/how-to-solve-no-visible-binding-for-global-variable-note/28887).
  • Rd line widths: Some lines exceed 100 characters. Adjusting your code style should address this:
 demo_metadata_file <- system.file(
 "inputs",
 "360_NationalCommunityChildHealthDatabase(NCCHD)_Structural_Metadata.csv",
 package = "mapmetadata"
 )

I hope these comments are helpful! Please don’t hesitate to reach out if anything requires further clarification.

Best regards,

Yohann

@RayStick
Copy link
Author

RayStick commented Jan 17, 2025

Thank you so much for your comments @ymansiaux ! 
I won’t work on them today (or until the other review comes in) but I have a quick question -

To align with the naming of the other exported functions, map_compare() and map_convert(), I would suggest renaming metadata_map() to map_metadata(). While I understand you may have concerns about potential confusion with the package name, I believe this change would harmonize the naming scheme, enhance clarity, and facilitate autocomplete for users.

I struggled with function naming and changed them a lot 😅
My logic here was following this guidance which says 'Consider an object_verb() naming scheme for functions in your package':

  • metadata_map because 'metadata' can be thought of the data object and 'map' the verb
  • map_convert because now the 'map' (well 'mappings') are the object, and the verb is 'convert'
  • map_compare now the 'map' (well 'mappings') are the object, an the verb is 'compare'

Does that make sense? I am open to changing them! On reflection, I am using map to be an object and a verb so that is confusing.

@ymansiaux
Copy link

Yes that perfectly makes sense :-) Considering the object_verb() naming scheme I think you should keep the current function names.

Have a good day !

@maelle
Copy link
Member

maelle commented Jan 17, 2025

Thanks so much @ymansiaux for your thorough review!

@maelle
Copy link
Member

maelle commented Jan 17, 2025

@ropensci-review-bot submit review #674 (comment) time 7

@ropensci-review-bot
Copy link
Collaborator

Logged review for ymansiaux (hours: 7)

@maelle
Copy link
Member

maelle commented Jan 17, 2025

As an alternative to @noRd you might enjoy https://github.com/moodymudskipper/devtag @RayStick (not in the dev guide yet), it generates Rd pages for internal functions, that developers can read with ?fun, but Rbuildignores them so the users don't see them.

@ropensci-review-bot
Copy link
Collaborator

📆 @Lextuga007 you have 2 days left before the due date for your review (2025-01-30).

@Lextuga007
Copy link

Lextuga007 commented Jan 28, 2025

Just to start by saying thank you for all the work you've put into this package 😃 Like @ymansiaux I learned a lot from the review and I hope my points are also helpful.

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

  • Briefly describe any working relationship you have (had) with the package authors.
  • 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
  • Examples: (that run successfully locally) for all exported functions
  • 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 have been confirmed.
  • Performance: Any performance claims of the software have 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.

Some changes are required, particularly for README and vignette documentation.

Estimated hours spent reviewing: 6.5 hours

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

Review Comments

Comprehensive tests for functions (both exported and internal) with every function having a test.
Good package name (checked with pak) which is descriptive.
Package has been built to interact as much as possible with the user and following that approach many of the subsequent comments are improvements from the documentation.
Really clear which functions are internal through the title.
Documentation fits the ROpenSci principle of multiple points of entry with README, vignettes and each function (including internal) documented.

ROpenSci packaging guidelines

Console messages asks to avoid the use of cat and print as these are not easy to suppress.

Avoiding verbosity or suppressing messages is hard within this package as the interactions from the user require the messages.

Accessibility

Thank you for the alt text on images!

To support further accessibility I recommend being more descriptive in the text where a link is highlighted in text like for the word here particularly as this is something that helps with accessibility.

Also appears in the vignette which shows an image but only says download here:.

It took me a while to navigate the website as I wasn't familiar with it so I wonder if the image would benefit from a direct hyperlink to the same page?

And the non-descriptive here in see here for outputs generated from the demo run.

Technical language

It might be useful to expand on the point about changing the save location by adjusting the output_dir argument because argument is a technical term that might not be familiar to researchers - perhaps using an example to expand on this would help?

Expectations of functionality

Only the Return key works to continue where the instruction says to finish here, or press any other key to continue with mapping variables.
Other keys don't appear to "finish" and also, to be very clear, need to be executed in the Console.

The demo requires initials to be entered but it isn't explained why it is required.
It gets recorded in the logs but this might not be apparent to an initial user of the package.

Demo mode is said to process only the first 20 variables from the selected table and it's not clear what the 20 variables refer to particularly when directed to selecting data elements.
Are data elements the same as variables in this example? I got a bit confused with this so couldn't complete the instructions.

The parameters used in the metadata_map() function example don't appear to work.
The code is metadata_map(csv_file = new_csv_file, domain_file = demo_domains_file) but there isn't a parameter called csv_file - should this be metadata_file?
I first of all tried to use the 14_Mortality (Death registration)_Metadata.csv data from the website but got the error because the use of structural metadata is only referred to in the making of metadata.
I found this link via the arguments help for metadata_file which is excellent documentation, however, will be missed I think so would be really great to be clear in the README file as well.
An enhancement may be to have a message coded if a file is missing structural in the file name that the error is because it's the wrong file?

get("look_up") should perhaps come earlier in the vignette because it's not possible to use this function when in the process of selecting variables from using metadata_map() that appears just before.

For the prompt Enter one or more numbers separated by spaces and then ENTER, or 0 to cancel I tried just using the Enter/Return key to finish which gave an output.
Pressing Enter/Return key though isn't a listed option and I just guessed it would work so possibly better to be explicit.

I recommend linking to the function reference for map_compare() in tutorial.

I recommend being explicit in the argument help for metadata_file as ?metadata won't return any files if the package hasn't been loaded: ?mapmetadata::metadata. Although this is functionality in RStudio it could be missed by someone new to R and RStudio and their experience of not finding help might be incorrectly associated with the package.

The reference to data/metadata.rda might not make sense as packages are often just used through the library() so perhaps better to use refer to the code mapmetadata::metadata?

For Using a custom domain list input where domain file inputs are detailed as automatically appending so do not include these in your domain list is this because they get replicated? It would be helpful to be explicit that this wouldn't be likely to break code but just duplicate.

For Using a custom lookup table input (advanced) I couldn't work out the corresponding example csv that the instruction ensure that all domain codes in the lookup file are also included in your *domain file* without opening all the csvs and looking for the column name.
It might be good to expand on this with an example.

Function documentation sometimes includes references to other functions in the package, for example valid_comparison() refers to it being used in the the map_compare.R function. Cross links are mentioned in the ROpenSci package guidelines and if you use RStudio I recommend using my colleague, Matt Dray's, snorkel package to add these in as you can highlight the text and use the RStudio add in.

Spelling

Minor spelling have been submitted for correction on a PR but where functions have spelling mistakes:

concensus is a function name and will require updates to corresponding files: concensus_on_mismatch(), test-concensus_on_mismatch.R.

It is also in the message from the map_compare() function and is in the documentation for map_compare().

Object in function metadata_map() (which won't have any effect to the user) mistmatch.

Package build

Notes on no visible binding for global variable can be managed using the utils package and a specific R file (globals.R). More details on more sophisticated treatment can be found in the R packages book. I tend to get these issues particularly when referring to SQL columns in R code.

portable file names... NOTE is appearing because the file names are over 100 characters long.
These files are referred to in the map_convert() and map_compare() documentation example so it might be ok to rename this longer name by the parts for generalising? For example:

#' @examples
#' # Locate file path and file name for the example files in the package
#' demo_output_dir <- system.file("outputs", package = "mapmetadata")
#' demo_output_csv <- "MAPPING_360_example_datetime.csv"
#'
#' # Run the function
#' map_convert(output_csv = demo_output_csv, output_dir = demo_output_dir)

Formatting consistency

I noticed that there are line breaks put into function metadata_map.R so almost all the text is within an 80 character limit, but this isn't the case for other scripts.
It's quite nice to have a line break set because it makes it easier to read across the screen and as it's been used in one script I wondered if it could be applied to all scripts?
I use lintr to highlight the lines that are over 80 characters.

@ymansiaux shared the devtools::check output which is useful as I don't get all the same notes (I'm missing the line widths note) so for comparison this is mine:

══ Documenting ══════════════════════════════════════════════════
ℹ Updating mapmetadata documentation
ℹ Loading mapmetadata

══ Building ═════════════════════════════════════════════════════
Setting env vars:
• CFLAGS    : -Wall -pedantic -fdiagnostics-color=always
• CXXFLAGS  : -Wall -pedantic -fdiagnostics-color=always
• CXX11FLAGS: -Wall -pedantic -fdiagnostics-color=always
• CXX14FLAGS: -Wall -pedantic -fdiagnostics-color=always
• CXX17FLAGS: -Wall -pedantic -fdiagnostics-color=always
• CXX20FLAGS: -Wall -pedantic -fdiagnostics-color=always
── R CMD build ──────────────────────────────────────────────────
✔  checking for file 'C:\Users\zoe.turner\OneDrive - Midlands and Lancashire CSU\Documents\4. GitHub\mapmetadata/DESCRIPTION' (403ms)
─  preparing 'mapmetadata': (536ms)
✔  checking DESCRIPTION meta-information ...
─  installing the package to build vignettes
✔  creating vignettes (9.8s)
─  checking for LF line-endings in source and make files and shell scripts (1.3s)
─  checking for empty or unneeded directories
─  building 'mapmetadata_3.0.0.tar.gz'
   Warning in utils::tar(filepath, pkgname, compression = compression, compression_level = 9L,  :
     storing paths of more than 100 bytes is not portable:
     'mapmetadata/inst/outputs/BAR_360_NationalCommunityChildHealthDatabase(NCCHD)_2024-12-19-14-11-55.html'
   Warning in utils::tar(filepath, pkgname, compression = compression, compression_level = 9L,  :
     storing paths of more than 100 bytes is not portable:
     'mapmetadata/inst/outputs/CONSENSUS_MAPPING_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-38-45.csv'
   Warning in utils::tar(filepath, pkgname, compression = compression, compression_level = 9L,  :
     storing paths of more than 100 bytes is not portable:
     'mapmetadata/inst/outputs/L-MAPPING_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-11-55.csv'
   Warning in utils::tar(filepath, pkgname, compression = compression, compression_level = 9L,  :
     storing paths of more than 100 bytes is not portable:
     'mapmetadata/inst/outputs/MAPPING_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-11-55.csv'
   Warning in utils::tar(filepath, pkgname, compression = compression, compression_level = 9L,  :
     storing paths of more than 100 bytes is not portable:
     'mapmetadata/inst/outputs/MAPPING_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-17-45.csv'
   Warning in utils::tar(filepath, pkgname, compression = compression, compression_level = 9L,  :
     storing paths of more than 100 bytes is not portable:
     'mapmetadata/inst/outputs/MAPPING_LOG_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-11-55.csv'
   Warning in utils::tar(filepath, pkgname, compression = compression, compression_level = 9L,  :
     storing paths of more than 100 bytes is not portable:
     'mapmetadata/inst/outputs/MAPPING_LOG_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-17-45.csv'
   Warning in utils::tar(filepath, pkgname, compression = compression, compression_level = 9L,  :
     storing paths of more than 100 bytes is not portable:
     'mapmetadata/inst/outputs/MAPPING_PLOT_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-11-55.png'
   
══ Checking ═════════════════════════════════════════════════════
Setting env vars:
• _R_CHECK_CRAN_INCOMING_REMOTE_               : FALSE
• _R_CHECK_CRAN_INCOMING_                      : FALSE
• _R_CHECK_FORCE_SUGGESTS_                     : FALSE
• _R_CHECK_PACKAGES_USED_IGNORE_UNUSED_IMPORTS_: FALSE
• NOT_CRAN                                     : true
── R CMD check ──────────────────────────────────────────────────
─  using log directory 'C:/Users/zoe.turner/AppData/Local/Temp/Rtmp4SNDF6/file83c42e8f7965/mapmetadata.Rcheck' (536ms)
─  using R version 4.4.1 (2024-06-14 ucrt)
─  using platform: x86_64-w64-mingw32
─  R was compiled by
       gcc.exe (GCC) 13.2.0
       GNU Fortran (GCC) 13.2.0
─  running under: Windows 10 x64 (build 19045)
─  using session charset: UTF-8
─  using options '--no-manual --as-cran'
✔  checking for file 'mapmetadata/DESCRIPTION' ...
─  checking extension type ... Package
─  this is package 'mapmetadata' version '3.0.0'
─  package encoding: UTF-8
✔  checking package namespace information
✔  checking package dependencies (1.6s)
✔  checking if this is a source package ...
✔  checking if there is a namespace
✔  checking for executable files (1.4s)
✔  checking for hidden files and directories ...
N  checking for portable file names
   Found the following non-portable file paths:
     mapmetadata/inst/outputs/BAR_360_NationalCommunityChildHealthDatabase(NCCHD)_2024-12-19-14-11-55.html
     mapmetadata/inst/outputs/CONSENSUS_MAPPING_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-38-45.csv
     mapmetadata/inst/outputs/L-MAPPING_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-11-55.csv
     mapmetadata/inst/outputs/MAPPING_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-11-55.csv
     mapmetadata/inst/outputs/MAPPING_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-17-45.csv
     mapmetadata/inst/outputs/MAPPING_LOG_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-11-55.csv
     mapmetadata/inst/outputs/MAPPING_LOG_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-17-45.csv
     mapmetadata/inst/outputs/MAPPING_PLOT_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-11-55.png
   
   Tarballs are only required to store paths of up to 100 bytes and cannot
   store those of more than 256 bytes, with restrictions including to 100
   bytes for the final component.
   See section 'Package structure' in the 'Writing R Extensions' manual.
✔  checking serialization versions
✔  checking whether package 'mapmetadata' can be installed (6.2s)
N  checking installed package size (443ms)
     installed size is  5.3Mb
     sub-directories of 1Mb or more:
       outputs   4.4Mb
✔  checking package directory (378ms)
✔  checking for future file timestamps ...
✔  checking 'build' directory
✔  checking DESCRIPTION meta-information (394ms)
✔  checking top-level files
✔  checking for left-over files ...
✔  checking index information (384ms)
✔  checking package subdirectories (1.3s)
✔  checking code files for non-ASCII characters ... 
✔  checking R files for syntax errors ... 
✔  checking whether the package can be loaded (1.8s)
✔  checking whether the package can be loaded with stated dependencies (962ms)
✔  checking whether the package can be unloaded cleanly (1.3s)
✔  checking whether the namespace can be loaded with stated dependencies (997ms)
✔  checking whether the namespace can be unloaded cleanly (1.2s)
✔  checking dependencies in R code (2.3s)
✔  checking S3 generic/method consistency (1.2s)
✔  checking replacement functions (992ms)
✔  checking foreign function calls (1.1s)
N  checking R code for possible problems (8.7s)
   empty_count: no visible binding for global variable 'Section'
   empty_count: no visible binding for global variable
     'Column.description'
   empty_count: no visible binding for global variable 'total_variables'
   empty_count: no visible binding for global variable 'no_count'
   empty_count: no visible binding for global variable 'Empty'
   end_plot: no visible binding for global variable 'domain_code'
   map_compare: no visible binding for global variable 'Section'
   map_compare: no visible binding for global variable 'data_element'
   map_compare: no visible global function definition for 'contains'
   metadata_map: no visible binding for global variable 'Section'
   metadata_map: no visible binding for global variable 'note'
   output_copy: no visible binding for global variable 'data_element'
   Undefined global functions or variables:
     Column.description Empty Section contains data_element domain_code
     no_count note total_variables
✔  checking Rd files (533ms)
✔  checking Rd metadata ... 
N  checking Rd line widths (342ms)
   Rd file 'map_compare.Rd':
     \examples lines wider than 100 characters:
        demo_metadata_file <- system.file("inputs", "360_NationalCommunityChildHealthDatabase(NCCHD)_Structural_Metadata.csv", package = "mapme ... [TRUNCATED]
   
   Rd file 'map_convert.Rd':
     \examples lines wider than 100 characters:
        demo_output_csv <- "MAPPING_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-17-45.csv"
   
   These lines will be truncated in the PDF manual.
✔  checking Rd cross-references ... 
✔  checking for missing documentation entries (1.1s)
✔  checking for code/documentation mismatches (3.7s)
✔  checking Rd \usage sections (1.8s)
✔  checking Rd contents ... 
✔  checking for unstated dependencies in examples ... 
✔  checking contents of 'data' directory ...
✔  checking data for non-ASCII characters (445ms)
✔  checking LazyData
✔  checking data for ASCII and uncompressed saves ... 
✔  checking installed files from 'inst/doc' (410ms)
✔  checking files in 'vignettes' (435ms)
✔  checking examples (1.9s)
✔  checking for unstated dependencies in 'tests' ... 
─  checking tests ...
✔  Running 'testthat.R' (4.7s)
✔  checking for unstated dependencies in vignettes (666ms)
✔  checking package vignettes ... 
✔  checking re-building of vignette outputs (5.6s)
✔  checking for non-standard things in the check directory
✔  checking for detritus in the temp directory
   
   See
     'C:/Users/zoe.turner/AppData/Local/Temp/Rtmp4SNDF6/file83c42e8f7965/mapmetadata.Rcheck/00check.log'
   for details.
   
── R CMD check results ───────────────────────────────────────────────────────────────────────────────────────────────────── mapmetadata 3.0.0 ────
Duration: 1m 0.7s

❯ checking for portable file names ... NOTE
  Found the following non-portable file paths:
    mapmetadata/inst/outputs/BAR_360_NationalCommunityChildHealthDatabase(NCCHD)_2024-12-19-14-11-55.html
    mapmetadata/inst/outputs/CONSENSUS_MAPPING_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-38-45.csv
    mapmetadata/inst/outputs/L-MAPPING_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-11-55.csv
    mapmetadata/inst/outputs/MAPPING_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-11-55.csv
    mapmetadata/inst/outputs/MAPPING_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-17-45.csv
    mapmetadata/inst/outputs/MAPPING_LOG_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-11-55.csv
    mapmetadata/inst/outputs/MAPPING_LOG_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-17-45.csv
    mapmetadata/inst/outputs/MAPPING_PLOT_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-11-55.png
  
  Tarballs are only required to store paths of up to 100 bytes and cannot
  store those of more than 256 bytes, with restrictions including to 100
  bytes for the final component.
  See section 'Package structure' in the 'Writing R Extensions' manual.

❯ checking installed package size ... NOTE
    installed size is  5.3Mb
    sub-directories of 1Mb or more:
      outputs   4.4Mb

❯ checking R code for possible problems ... NOTE
  empty_count: no visible binding for global variable 'Section'
  empty_count: no visible binding for global variable
    'Column.description'
  empty_count: no visible binding for global variable 'total_variables'
  empty_count: no visible binding for global variable 'no_count'
  empty_count: no visible binding for global variable 'Empty'
  end_plot: no visible binding for global variable 'domain_code'
  map_compare: no visible binding for global variable 'Section'
  map_compare: no visible binding for global variable 'data_element'
  map_compare: no visible global function definition for 'contains'
  metadata_map: no visible binding for global variable 'Section'
  metadata_map: no visible binding for global variable 'note'
  output_copy: no visible binding for global variable 'data_element'
  Undefined global functions or variables:
    Column.description Empty Section contains data_element domain_code
    no_count note total_variables

❯ checking Rd line widths ... NOTE
  Rd file 'map_compare.Rd':
    \examples lines wider than 100 characters:
       demo_metadata_file <- system.file("inputs", "360_NationalCommunityChildHealthDatabase(NCCHD)_Structural_Metadata.csv", package = "mapme ... [TRUNCATED]
  
  Rd file 'map_convert.Rd':
    \examples lines wider than 100 characters:
       demo_output_csv <- "MAPPING_360_NationalCommunityChildHealthDatabase(NCCHD)_CHILD_2024-12-19-14-17-45.csv"
  
  These lines will be truncated in the PDF manual.

0 errors ✔ | 0 warnings ✔ | 4 notes ✖

And my session info:

R version 4.4.1 (2024-06-14 ucrt)
Platform: x86_64-w64-mingw32/x64
Running under: Windows 10 x64 (build 19045)

Matrix products: default


locale:
[1] LC_COLLATE=English_United Kingdom.utf8  LC_CTYPE=English_United Kingdom.utf8    LC_MONETARY=English_United Kingdom.utf8
[4] LC_NUMERIC=C                            LC_TIME=English_United Kingdom.utf8    

time zone: Europe/London
tzcode source: internal

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

other attached packages:
[1] mapmetadata_3.0.0

loaded via a namespace (and not attached):
 [1] gtable_0.3.6      xfun_0.50         ggplot2_3.5.1     htmlwidgets_1.6.4 devtools_2.4.5    remotes_2.5.0     processx_3.8.5   
 [8] callr_3.7.6       vctrs_0.6.5       tools_4.4.1       ps_1.8.1          generics_0.1.3    curl_6.1.0        tibble_3.2.1     
[15] pkgconfig_2.0.3   data.table_1.16.4 xopen_1.0.1       desc_1.4.3        lifecycle_1.0.4   compiler_4.4.1    stringr_1.5.1    
[22] munsell_0.5.1     httpuv_1.6.15     htmltools_0.5.8.1 usethis_3.1.0     lazyeval_0.2.2    plotly_4.10.4     later_1.4.1      
[29] pillar_1.10.1     urlchecker_1.0.1  tidyr_1.3.1       ellipsis_0.3.2    cachem_1.1.0      sessioninfo_1.2.2 mime_0.12        
[36] tidyselect_1.2.1  digest_0.6.37     stringi_1.8.4     dplyr_1.1.4       purrr_1.0.2       rprojroot_2.0.4   fastmap_1.2.0    
[43] grid_4.4.1        colorspace_2.1-1  cli_3.6.3         magrittr_2.0.3    pkgbuild_1.4.6    withr_3.0.2       prettyunits_1.2.0
[50] scales_1.3.0      promises_1.3.2    roxygen2_7.3.2    httr_1.4.7        gridExtra_2.3     memoise_2.0.1     shiny_1.10.0     
[57] evaluate_1.0.3    knitr_1.49        rcmdcheck_1.4.0   miniUI_0.1.1.1    viridisLite_0.4.2 profvis_0.4.0     rlang_1.1.5      
[64] Rcpp_1.0.14       xtable_1.8-4      glue_1.8.0        xml2_1.3.6        pkgload_1.4.0     rstudioapi_0.17.1 jsonlite_1.8.9   
[71] R6_2.5.1          fs_1.6.5         

@maelle
Copy link
Member

maelle commented Jan 29, 2025

Thanks a lot @Lextuga007 for your thorough review! 🚀

Regarding verbosity, we've published a blog post a while ago (with the goal of updating the dev guide later), that might be relevant: https://ropensci.org/blog/2024/02/06/verbosity-control-packages/

@RayStick Both reviews are now in. 😸

@maelle
Copy link
Member

maelle commented Jan 29, 2025

@ropensci-review-bot submit review #674 (comment) time 6.5

@ropensci-review-bot
Copy link
Collaborator

Logged review for Lextuga007 (hours: 6.5)

@Lextuga007
Copy link

@maelle thanks for sharing the blog - that's really useful for my own package development as I'm just puzzling out the best way to give messages and get them recorded for internal data pipeline packages😀

@maurolepore
Copy link
Member

Dear @RayStick this is to mark the start of my EiC rotation. I'm reviewing all open issues and leaving a short note to myself of what I see.

I see both reviews are in and I assume you're working on incorporating the feedback. Everything seems on track. I'll step back. Thanks!

@RayStick
Copy link
Author

RayStick commented Feb 3, 2025

Hi @maurolepore yes I am working on incorporating the feedback, thanks =D

@RayStick
Copy link
Author

RayStick commented Feb 3, 2025

@maelle what is the etiquette with asking reviewers to review pull requests when I make some of their suggested changes? Is it best to just make the change to the best of my ability, and link to all the changes in one go at the end?

@ropensci-review-bot
Copy link
Collaborator

@RayStick, @BatoolMM, @Rainiefantasy: please post your response with @ropensci-review-bot submit response <url to issue comment> if you haven't done so already (this is an automatic reminder).

Here's the author guide for response. https://devguide.ropensci.org/authors-guide.html

@maelle
Copy link
Member

maelle commented Feb 10, 2025

Is it best to just make the change to the best of my ability, and link to all the changes in one go at the end?

Sorry for the delay. Yes it is best to do it this way.

@RayStick
Copy link
Author

RayStick commented Feb 10, 2025

@maelle I am aiming for end of this week to address all the reviewer's comments. Sorry for the delay, I have had a more hectic couple of work weeks than expected. Is that okay?

Progress is here: aim-rsf/mapmetadata#184 (but I will make to summarise and itemise in this issue thread as per the guidelines, once I am done)

@maelle
Copy link
Member

maelle commented Feb 11, 2025

Yes it is ok @RayStick, no worries.

Note that once you post your response, you can "submit" it with a bot command so the label of the issue will change: https://devguide.ropensci.org/bot_cheatsheet.html#submit-response-to-reviewers

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