software-review
software-review copied to clipboard
fellingdateR: Estimate, report and combine felling dates of historical tree-ring series
Submitting Author Name: Kristof Haneca Submitting Author Github Handle: @hanecakr Other Package Authors Github handles: (comma separated, delete if none) Repository: https://github.com/hanecakr/fellingdateR Version submitted: Submission type: Standard Editor: @maelle Reviewers: @njtierney, @ajpelu
Archive: TBD Version accepted: TBD Language: en
- Paste the full DESCRIPTION file inside a code block below:
Package: fellingdateR
Title: Estimate, report and combine felling dates of historical tree-ring
series
Version: 0.0.0.9003
Authors@R: c(
person("Kristof", "Haneca", , "[email protected]",role = c("aut", "cre"),
comment = c(ORCID = "0000-0002-7719-8305")),
person("Koen", "Van Daele", role = "ctb",
comment = c(ORCID = "0000-0002-8153-2978")),
person("Ronald", "Visser", role = "ctb",
comment = c(ORCID = "0000-0001-6966-1729"))
)
Description: `fellingdateR` is an R package that aims to facilitate the
analysis and interpretation of tree-ring data from wooden
cultural heritage objects and structures. The package standarizes the process
of computing and combining felling date estimates, both for individual and
groups of related tree-ring series.
URL: https://github.com/hanecakr/fellingdateR
BugReports: https://github.com/hanecakr/fellingdateR/issues
License: MIT + file LICENSE
Encoding: UTF-8
Roxygen: list(markdown = TRUE)
RoxygenNote: 7.2.3
Depends:
R (>= 2.10)
LazyData: true
Imports:
dplyr,
ggplot2,
ggtext,
MASS,
plyr,
tidyr,
utils,
dplR
Suggests:
covr,
knitr,
rmarkdown,
testthat (>= 3.0.0)
Config/testthat/edition: 3
VignetteBuilder: knitr
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
- [x] 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):
The package aims to facilitate and standardize the computation of felling dates from dated tree-ring series. It covers all steps of processing measured ring-width series from raw data to the reporting of felling date estimates, for single series and for groups of related tree-ring series.
- Who is the target audience and what are scientific applications of this package?
Maily professional tree-ring scientists (dendrochronologists) that work on historical timbers and wooden objects (archaeology, architectural history, sculptures, panel paintings, etc.)
- Are there other R packages that accomplish the same thing? If so, how does yours differ or meet our criteria for best-in-category?
No, no other R-packages do the same job.
- (If applicable) Does your package comply with our guidance around Ethics, Data Privacy and Human Subjects Research?
yes
-
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.
All checks pass
Technical checks
Confirm each of the following by checking the box.
- [x ] I have read the rOpenSci packaging guide.
- [x ] I have read the author guide and I expect to maintain this package for at least 2 years or to find a replacement.
This package:
- [x] does not violate the Terms of Service of any service it interacts with.
- [x] has a CRAN and OSI accepted license.
- [x] contains a README with instructions for installing the development version.
- [x] includes documentation with examples for all functions, created with roxygen2.
- [x] contains a vignette with examples of its essential functions and uses.
- [x] has a test suite.
- [x] has continuous integration, including reporting of test coverage.
Publication options
-
[x] Do you intend for this package to go on CRAN?
-
[ ] Do you intend for this package to go on Bioconductor?
-
[ ] Do you wish to submit an Applications Article about your package to Methods in Ecology and Evolution? If so:
MEE Options
- [ ] The package is novel and will be of interest to the broad readership of the journal.
- [ ] The manuscript describing the package is no longer than 3000 words.
- [ ] You intend to archive the code for the package in a long-term repository which meets the requirements of the journal (see MEE's Policy on Publishing Code)
- (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
- [x] I agree to abide by rOpenSci's Code of Conduct during the review process and in maintaining my package should it be accepted.
Thanks for submitting to rOpenSci, our editors and @ropensci-review-bot will reply soon. Type @ropensci-review-bot help
for help.
:rocket:
The following problem was found in your submission template:
- URL = [https] is not valid
The package could not be checked because of problems with the URL.
Editors: Please ensure these problems are rectified, and then call
@ropensci-review-bot check package
.
:wave:
@hanecakr The error was because your URL field was markdown-style: "[url](url)", and should just be plain "url". I've edited to fix; you should now be able to call @ropensci-review-bot check package
yourself to restart checks. Sorry for any inconvencience.
@ropensci-review-bot check package
Thanks, about to send the query.
:rocket:
Editor check started
:wave:
Checks for fellingdateR (v0.0.0.9003)
git hash: 05c449fb
- :heavy_check_mark: Package name is available
- :heavy_check_mark: has a 'codemeta.json' file.
- :heavy_check_mark: has a 'contributing' file.
- :heavy_check_mark: uses 'roxygen2'.
- :heavy_check_mark: 'DESCRIPTION' has a URL field.
- :heavy_check_mark: 'DESCRIPTION' has a BugReports field.
- :heavy_check_mark: Package has at least one HTML vignette
- :heavy_check_mark: All functions have examples.
- :heavy_check_mark: Package has continuous integration checks.
- :heavy_check_mark: Package coverage is 77.6%.
- :heavy_check_mark: R CMD check found no errors.
- :heavy_check_mark: R CMD check found no warnings.
- :eyes: Function names are duplicated in other packages
(Checks marked with :eyes: may be optionally addressed.)
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 | 363 |
internal | stats | 33 |
internal | fellingdateR | 26 |
internal | grDevices | 7 |
internal | graphics | 2 |
imports | ggplot2 | 32 |
imports | utils | 15 |
imports | dplyr | 6 |
imports | ggtext | 3 |
imports | plyr | 2 |
imports | tidyr | 2 |
imports | dplR | 2 |
imports | MASS | 1 |
suggests | covr | NA |
suggests | knitr | NA |
suggests | rmarkdown | NA |
suggests | testthat | 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
sub (32), rep (31), length (21), c (17), nrow (17), apply (15), matrix (15), max (14), ncol (13), data.frame (10), seq (10), which (10), range (9), for (8), paste0 (8), as.data.frame (7), attributes (7), numeric (7), as.numeric (6), by (6), drop (5), is.na (5), mean (5), min (5), list (4), summary (4), diff (3), floor (3), format (3), rownames (3), setdiff (3), sum (3), T (3), unique (3), any (2), character (2), get (2), grep (2), lapply (2), names (2), round (2), sapply (2), scale (2), seq_len (2), abs (1), all (1), as.character (1), as.integer (1), ceiling (1), colnames (1), colSums (1), complex (1), cumsum (1), dim (1), file.exists (1), gettext (1), Im (1), intersect (1), lengths (1), logical (1), match (1), merge (1), message (1), Re (1), readLines (1), regexpr (1), seq_along (1), sort (1), sqrt (1), substring (1), table (1), vapply (1)
stats
df (18), spline (7), end (3), sd (2), start (2), sigma (1)
ggplot2
element_blank (13), element_text (5), ggplot (3), aes (2), arrow (2), geom_line (2), unit (2), geom_area (1), scale_x_continuous (1), theme (1)
fellingdateR
hdi (7), sw_model (6), sw_data_overview (3), d.count (2), d.dens (2), fd_report (2), sw_interval (2), strip.comment (1), sw_combine_plot (1)
utils
data (13), citation (1), tail (1)
grDevices
pdf (7)
dplyr
left_join (1), mutate (1), pull (1), relocate (1), select (1), summarize (1)
ggtext
element_markdown (3)
dplR
rwl.stats (2)
graphics
title (2)
plyr
round_any (2)
tidyr
pivot_longer (2)
MASS
fitdistr (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 16 files) and
- 1 authors
- 1 vignette
- 13 internal data files
- 8 imported packages
- 15 exported functions (median 80 lines of code)
- 21 non-exported functions in R (median 50 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 | 16 | 74.9 | |
files_vignettes | 1 | 68.4 | |
files_tests | 14 | 93.8 | |
loc_R | 2855 | 89.8 | |
loc_vignettes | 193 | 48.1 | |
loc_tests | 480 | 74.1 | |
num_vignettes | 1 | 64.8 | |
data_size_total | 6023 | 68.2 | |
data_size_median | 465 | 60.0 | |
n_fns_r | 36 | 45.9 | |
n_fns_r_exported | 15 | 58.5 | |
n_fns_r_not_exported | 21 | 42.1 | |
n_fns_per_file_r | 1 | 20.7 | |
num_params_per_fn | 4 | 67.3 | |
loc_per_fn_r | 65 | 93.5 | |
loc_per_fn_r_exp | 80 | 89.2 | |
loc_per_fn_r_not_exp | 50 | 90.3 | |
rel_whitespace_R | 11 | 80.9 | |
rel_whitespace_vignettes | 68 | 73.3 | |
rel_whitespace_tests | 11 | 58.1 | |
doclines_per_fn_exp | 36 | 43.7 | |
doclines_per_fn_not_exp | 0 | 0.0 | TRUE |
fn_call_network_size | 30 | 55.5 |
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
GitHub Workflow Results
id | name | conclusion | sha | run_number | date |
---|---|---|---|---|---|
6932735657 | pkgcheck | success | 05c449 | 5 | 2023-11-20 |
6932735666 | R-CMD-check | success | 05c449 | 9 | 2023-11-20 |
6932735656 | test-coverage | success | 05c449 | 12 | 2023-11-20 |
3b. goodpractice
results
R CMD check
with rcmdcheck
R CMD check generated the following note:
- checking data for non-ASCII characters ... NOTE Note: found 11 marked UTF-8 strings
R CMD check generated the following check_fails:
- cyclocomp
- rcmdcheck_non_ascii_characters_in_data
Test coverage with covr
Package coverage: 77.6
Cyclocomplexity with cyclocomp
The following functions have cyclocomplexity >= 15:
function | cyclocomplexity |
---|---|
read_fh | 150 |
fd_report | 42 |
sw_sum | 32 |
cor_table | 31 |
sw_combine | 27 |
sw_interval | 17 |
movAv | 16 |
sw_combine_plot | 15 |
Static code analyses with lintr
lintr found the following 170 potential issues:
message | number of times |
---|---|
Avoid 1:length(...) expressions, use seq_len. | 6 |
Avoid library() and require() calls in packages | 1 |
Avoid using sapply, consider vapply instead, that's type safe | 2 |
Lines should not be more than 80 characters. | 154 |
Use <-, not =, for assignment. | 7 |
4. Other Checks
Details of other checks (click to open)
:heavy_multiplication_x: The following 3 function names are duplicated in other packages:
-
-
get_header
from eventr
-
-
-
hdi
from bayestestR, ggdist, hdi, HDInterval
-
-
-
movAv
from berryFunctions
-
Package Versions
package | version |
---|---|
pkgstats | 0.1.3.9 |
pkgcheck | 0.1.2.11 |
Editor-in-Chief Instructions:
This package is in top shape and may be passed on to a handling editor
@hanecakr Thank you for the submission. All looks good to me. Will pass on the handling editor momentarily.
@ropensci-review-bot assign @maelle as editor
Assigned! @maelle is now the editor
Thanks for your submission @hanecakr! I have some comments before I can proceed with the reviewer search. :deciduous_tree:
Happy to discuss the comments below! :smile_cat:
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,
- [x] 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?
-
[ ] 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?
-
[x] Contributing information: Is the documentation for contribution clear enough e.g. tokens for tests, playgrounds?
-
[x] License: The package has a CRAN or OSI accepted license.
-
[x] 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
Installation instructions
For brevity you can keep only the installation instructions with pak.
Dependencies
I wonder whether the dependency on plyr for very few functions could be avoided?
Metadata
I don't think it's allowed to have backticks in a DESCRIPTION file, I see some in the Description field. Or if it's allowed, at least it's unusual?
Docs
I'd recommend creating a pkgdown website. In the vignette and README it'd make sense to remove the description of functions and datasets, instead pointing to the reference index of the pkgdown website. It'd make it easier to skim the README, and to see what functions/datasets there are at once (for reviewers but also users!).
Data
I see the example datasets are saved as internal datasets however you use them in the documentation (in the README) so why not make them exported datasets? https://r-pkgs.org/data.html#sec-data-data
Rather than using the names with "dummy" I'd recommend informative names. (Beside, some might consider "dummy" to be an offensive word related to ableism).
#### Tests
I see some top-level code in for instance https://github.com/hanecakr/fellingdateR/blob/master/tests/testthat/test-fd_report.R I'd recommend instead having a function defined in a helper file like tests/testthat/helper-testdata
test_data <- function() {
data.frame(series = c("aaa", "bbb", "ccc", "no_last", "no_sapwood"),
n_sapwood = c(10, 11, 12, 10, NA),
waneyedge = c(FALSE, FALSE, TRUE, FALSE, FALSE),
last = c(123, 456, 1789, NA, 1978))
}
that you'd then call from each test (not each test file) to create the data. Even better, it could have a more informative name. For more context: https://blog.r-hub.io/2020/11/18/testthat-utility-belt/#code-called-in-your-tests and https://r-pkgs.org/testing-advanced.html#sec-testing-advanced-fixture-helper With such a strategy, each test is easier to read and to run, as it's self-contained.
I think some test lines could be more condensed, e.g. https://github.com/hanecakr/fellingdateR/blob/05c449fb13b678ac1700177d73a20fad6c55da8c/tests/testthat/test-fd_report.R#L105 could go on the previous line, and https://github.com/hanecakr/fellingdateR/blob/05c449fb13b678ac1700177d73a20fad6c55da8c/tests/testthat/test-fd_report.R#L96-L98 or https://github.com/hanecakr/fellingdateR/blob/05c449fb13b678ac1700177d73a20fad6c55da8c/tests/testthat/test-fd_report.R#L120-L122 might be a single line, to see more of the test file at once on the screen?
You shouldn't namespace fellingdateR:::
data in test files.
For test-sw_combine_plot.R
you might want to look into snapshot testing rather than using ggplot2's internal data structure: https://testthat.r-lib.org/articles/snapshotting.html#whole-file-snapshotting (other relevant source: https://www.tidyverse.org/blog/2022/09/playing-on-the-same-team-as-your-dependecy/#testing-testing)
Why is there a test that is commented out? https://github.com/hanecakr/fellingdateR/blob/05c449fb13b678ac1700177d73a20fad6c55da8c/tests/testthat/test-sw_interval.R#L2 (as opposed to, say, completely removed)
Instead of the regular expressions for testing errors, you might be interested in this approach of classed errors: https://www.mm218.dev/posts/2023-11-07-classed-errors/
Code
Please read the output from lintr in the automatic checks above, and fix accordingly (or update this thread to explain why you disagree).
Lines such as https://github.com/hanecakr/fellingdateR/blob/05c449fb13b678ac1700177d73a20fad6c55da8c/R/cor_table.R#L117 should use inherits()
if (!inherits(x, "rwl")) {
In for instance cor_table.R
, I think the code might be more readable with explaining variables. For instance
if (!all(diff(as.numeric(row.names(x))) == 1)) {
would be
increasing_consecutive_years <- (all(diff(as.numeric(row.names(x))) == 1))
if (!increasing_consecutive_years) {
Same comments as for test files, I think there could be fewer new lines in scripts such as https://github.com/hanecakr/fellingdateR/blob/master/R/cor_table.R, to facilitate reading / vertical scrolling. I guess this conflicts a bit with the huge indentations in the files (which you are obviously allowed to prefer!).
For the else if in https://github.com/hanecakr/fellingdateR/blob/05c449fb13b678ac1700177d73a20fad6c55da8c/R/sw_interval.R#L236, why not use the switch()
function? I must confess I like using it but have to look up the docs every time to remember how to input the arguments. But it'll decrease the complexity of the current logic.
In https://github.com/hanecakr/fellingdateR/blob/05c449fb13b678ac1700177d73a20fad6c55da8c/R/sw_interval_plot.R#L137 what warnings are suppressed? In the same post mentioned previously https://www.mm218.dev/posts/2023-11-07-classed-errors/, it's shown that one can selectively suppress warnings of a given class.
Thanks a lot @maelle for this first round of comments. Will try to tackle most of these issues in the next few days.
Dear @maelle. I've been working on the package and think I'm able now to address most of your comments. Thanks a lot for your time, valuable insights and recommendations!
I'll list the answers to your comments here:
Installation instructions
For brevity you can keep only the installation instructions with pak.
- [x ] Ok. I've removed the install with devtools from README
Dependencies
I wonder whether the dependency on plyr for very few functions could be avoided?
- [x ] The function
plyr::round_any
is now removed fromsw_interval_plot()
,sw_combine_plot()
andsw_sum_plot()
- [x ]
floor/ceiling(x/10)*10
now does the job
Metadata
I don't think it's allowed to have backticks in a DESCRIPTION file, I see some in the Description field. Or if it's allowed, at least it's unusual?
- [x ] Backticks removed from DESCRIPTION
Docs
I'd recommend creating a pkgdown website.
- [x ] Initiated a pkgdown website now. I had postponed this because I also have a short paper ready (after code-review) for JOSS. Planned to use that paper (and reviewer comments) for an article/vignette on a pkgdown site. But obviously, for reviewers a pkgdown-site is most helpful at this stage already.
Data
I see the example datasets are saved as internal datasets however you use them in the documentation (in the README) so why not make them exported datasets? https://r-pkgs.org/data.html#sec-data-data
- [x ] The exported data are factual (published) data that are used by nearly all functions in the package. With
sw_data_overview()
users get an overview of what sapwood datasets are available in the package and withsw_data_info()
basic descriptives of those dataset. - [ ] When I would make the made-up date also external, that would mix real-world data and made-up data. And would also complicate the code for
sw_data_overview()
. Therefore I would like to keep the made-up data for examples and tests as internal data.
Rather than using the names with "dummy" I'd recommend informative names. (Beside, some might consider "dummy" to be an offensive word related to ableism).
- [x ] Thanks for pointing this out. Changed this throughout all functions and tests to “trs_example1” (tree-ring series example). Indeed much better than ‘dummy’ or ‘fake’.
Tests
I see some top-level code in for instance https://github.com/hanecakr/fellingdateR/blob/master/tests/testthat/test-fd_report.R I'd recommend instead having a function defined in a helper file like tests/testthat/helper-testdata ...
- [x ] The top-level code is in fact redundant. The test can equally be run with the internal data. This has now been implemented in the tests.
I think some test lines could be more condensed, e.g. ...
- [x ] I reformatted most test-files to make them more condensed. Should have done that before submission…
You shouldn't namespace fellingdateR::: data in test files.
- [x ] Right. Removed the namespace to fellingdateR in tests.
For test-sw_combine_plot.R you might want to look into snapshot testing rather than using ggplot2's internal data structure: https://testthat.r-lib.org/articles/snapshotting.html#whole-file-snapshotting (other relevant source: https://www.tidyverse.org/blog/2022/09/playing-on-the-same-team-as-your-dependecy/#testing-testing)
- [ ] I've left this as it is now. Can't find an elegant way to implement snapshot testing right now. Need to look deeper into that.
Why is there a test that is commented out? https://github.com/hanecakr/fellingdateR/blob/05c449fb13b678ac1700177d73a20fad6c55da8c/tests/testthat/test-sw_interval.R#L2 (as opposed to, say, completely removed)
- [x ] Oops, Removed this from the test-file
Instead of the regular expressions for testing errors, you might be interested in this approach of classed errors: https://www.mm218.dev/posts/2023-11-07-classed-errors/
Code
Please read the output from lintr in the automatic checks above, and fix accordingly (or update this thread to explain why you disagree).
Lines such as https://github.com/hanecakr/fellingdateR/blob/05c449fb13b678ac1700177d73a20fad6c55da8c/R/cor_table.R#L117 should use inherits() if (!inherits(x, "rwl")) {
- [x ] Now using inherits() in the latest version of
cor_table()
.
In for instance cor_table.R, I think the code might be more readable with explaining variables. For instance if (!all(diff(as.numeric(row.names(x))) == 1)) { would be increasing_consecutive_years <- (all(diff(as.numeric(row.names(x))) == 1)) if (!increasing_consecutive_years) {
- [x ] OK, I've changed this to increase readability
Same comments as for test files, I think there could be fewer new lines in scripts such as https://github.com/hanecakr/fellingdateR/blob/master/R/cor_table.R, to facilitate reading / vertical scrolling. I guess this conflicts a bit with the huge indentations in the files (which you are obviously allowed to prefer!).
- [x ] I’ve reformatted the code with RStudio > reformat code. Is indeed (a bit) more condensed now.
For the else if in https://github.com/hanecakr/fellingdateR/blob/05c449fb13b678ac1700177d73a20fad6c55da8c/R/sw_interval.R#L236, why not use the switch() function? I must confess I like using it but have to look up the docs every time to remember how to input the arguments. But it'll decrease the complexity of the current logic.
- [ ] I remember I tried this before, but failed to implement
switch()
for this function. Would prefer to leave it as it is now.
In https://github.com/hanecakr/fellingdateR/blob/05c449fb13b678ac1700177d73a20fad6c55da8c/R/sw_interval_plot.R#L137 what warnings are suppressed? In the same post mentioned previously https://www.mm218.dev/posts/2023-11-07-classed-errors/, it's shown that one can selectively suppress warnings of a given class.
- [x ] Doesn’t seem to be necessary anymore to use
suppressWarnings()
to avoid messages from ggplot, forsw_model()
andsw_combine()
. So I’ve removed the calls tosuppressWarnings()
for these functions. - [ ] For sw_interval() however I’ve kept the call to suppressWarnings to avoid warnings as:
Warning: Removed 52 rows containing non-finite values (stat_align()
).
Warning: Removed 43 rows containing missing values (geom_line()
).
I'm sure there are more elegant and effective ways to improve the scripts, functions and their documentation, but I hope that these initial revisions have elevated the quality of the package sufficiently, making it amenable for the review process. Of course, I'm open for additional insights and recommendations to further improved the quality and performance of the package.
-- Kristof
Thanks a ton @hanecakr!
A small note, in GitHub Markdown if you type - [x]
the checkbox appears as a ticked checkbox, whereas - [x ]
does not.
In the README when mentioning the reference index, you could rephrase the link so as not to use "here" (https://webaccess.berkeley.edu/ask-pecan/click-here), for instance "You can find the [list of functions]".
When I would make the made-up date also external, that would mix real-world data and made-up data. And would also complicate the code for sw_data_overview(). Therefore I would like to keep the made-up data for examples and tests as internal data.
Ok, but in that case would it work to not load sysdata (which might look confusing) but instead use data(dataname, package = "fellingdateR")
? Also to avoid using :::
when using the data in examples.
Should have done that before submission…
There's already a lot of stuff to do before submission!
Don't hesitate to ask if you need help with snapshot testing, but it's maybe not useful here anyway.
A small non compulsory thing, I see your default branch is named master, you could change it to the new community standard "main" using the usethis function for instance. See also https://www.tidyverse.org/blog/2021/10/renaming-default-branch/
Another Git thing, your repository contains the .Rproj.user folder, should it be removed and added to .gitignore? See https://usethis.r-lib.org/reference/git_vaccinate.html for a handy way to gitignore it globally.
I remember I tried this before, but failed to implement switch() for this function. Would prefer to leave it as it is now.
Fair enough! Happy to try and make a PR if you change your mind (maybe this time I'd remember how to use it straight away :joy: )
I'll now look for reviewers! Thanks again for all your work!
@ropensci-review-bot seeking reviewers
Please add this badge to the README of your package repository:
[![Status at rOpenSci Software Peer Review](https://badges.ropensci.org/618_status.svg)](https://github.com/ropensci/software-review/issues/618)
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 Thanks a lot!
Meanwhile, I will change the branch name to main
, use the data(...)
function to load internal data, and have a look at the .Rproj.user folder and try to find a way to add it to .gitignore. The badge will be added to README soon.
@ropensci-review-bot add @njtierney to reviewers
@njtierney added to the reviewers list. Review due date is 2023-12-22. Thanks @njtierney 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.
@njtierney: If you haven't done so, please fill this form for us to update our reviewers records.
Thank you @njtierney for accepting to review this package!
@njtierney: with a little delay, I decided to follow the advice of @maelle and moved the example datasets from internal to external data, so they become easily available for testing and examples, both for end-uses as during code review. The latest commit implements the necessary (small) changes. Looking forward to your comments and suggestions. -- Kristof.
Thanks, team! I've got a few deadlines this week but will take a look next week and aim to have this done by the 22nd.
Thank you @njtierney and good luck with the deadlines!
@ropensci-review-bot add @ajpelu to reviewers
@ajpelu added to the reviewers list. Review due date is 2023-12-30. Thanks @ajpelu 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.
@ajpelu: If you haven't done so, please fill this form for us to update our reviewers records.
Thank you @ajpelu for accepting to review this package!
:calendar: @njtierney you have 2 days left before the due date for your review (2023-12-22).
Hi team!
I'm still working through this review. I've done a first pass, but I am just finishing up collecting together my points and polishing up the review so it can be easy to follow. Should be done later this afternoon! :)