Comments and remove old test expectations.

R/community_generate.R

@@ -31,7 +31,7 @@ community_generate <- function(community_data_table, abundance_column_name = "sp
 
   community_vars <- colnames(community_data_table)
 
-  # Check that the necessary variables are provided
+  # Check that the necessary variables are provided ####
 
   contains_AOU <- "AOU" %in% community_vars
   contains_scientific_name <- "scientific_name" %in% community_vars
@@ -46,7 +46,7 @@ community_generate <- function(community_data_table, abundance_column_name = "sp
     stop("At least one of `AOU`, `scientific_name`, or `mean_size` is required")
   }
 
-  # Identify ID/grouping columns and columns to pass to sim fxns.
+  # Identify ID/grouping columns and columns to pass to sim fxns. ####
 
 
   community_data_table$rejoining_id <- seq_len(nrow(community_data_table))
@@ -62,7 +62,7 @@ community_generate <- function(community_data_table, abundance_column_name = "sp
 
   sim_vars <- c(community_vars_mod[which(community_vars_mod %in% possible_sim_vars)])
 
-  # # For the cols to pass in, add NA columns for any of the variables that the sim fxns can use that aren't included
+  # For the cols to pass in, add NA columns for any of the variables that the sim fxns can use that aren't included ####
   na_vars <- possible_sim_vars[which(!(possible_sim_vars %in% community_vars_mod))]
 
   na_table <- matrix(nrow = nrow(community_data_table), ncol = length(na_vars))

review_edits.md

@@ -328,3 +328,18 @@ I've also added sections right at the beginning of both the README and Getting S
 -   tests fail due to an issue with unzip on windows. 👀
 -   I've reached out to Dr. Dunning and he agreed to be listed as a Data Contributor. I've added him with this revision. 
 -   I reviewed the Small Fixes PR from @ mstrimas. Because of the scope of revisions, I ended up not merging that pull request and instead implementing the same changes on the revised codebase. Specifically, I tidied up the DESCRIPTION using desc::desc_normalize(), removed `remotes::install_github` from the README, switched T/F to TRUE/FALSE, and switched 1:nrow() to seq_len(nrow()). = was changed to <- via the lintr cleanup, and use of .data$ was removed with the removal of tidyverse functions.
+
+# From editor:
+
+* I'd recommend running desc::desc_normalize(): it will order DESCRIPTION fields in a standard way and order dependencies alphabetically. _Done!_
+*   You can remove the {remotes} installation instructions and keep the {devtools} ones only as {devtools} calls {remotes} (might call {pak} in the future?) and is the interface for users. _Done!_
+* It'd be nice to add grouping to the reference index https://pkgdown.r-lib.org/reference/build_reference.html#reference-index Given your package's naming scheme, you might want to use the starts_with() helper. _I haven't done this because there are so few functions, it seems to me like it verges on redundant. I'm happy to revisit this!_
+*    In the test filenames, why use numbers? If the R files and test files have the same basename, it's easier to navigate between the two in RStudio IDE. https://r-pkgs.org/testing-basics.html#test-organisation _Ah, I learned to use the numbers so that tests fail informatively in order. The tests also don't - all - correspond directly to RScripts. Is this a major barrier? If so, I'm happy to revisit!_
+*   In your test files and scripts I see a lot of vertical space, more than one empty lines between some elements. I'd recommend using one empty line only as it increases the amount of code one can see at a time on the screen. It's still nice to organize the code in paragraphs, I am not suggesting to remove all empty lines. 😁 _I  think this has been largely addressed via the re-styling I've done, but please let me know if the issue peresists!_
+*   In the README instead of "Package summary" as a header you could use the same text as this issue "birdsize: Estimate avian body size distributions" which is more informative. _Done!_
+*   Regarding data yes it might make sense to contact the author of the included dataset? _Done! I've reached out and Dr. Dunning agreed to be listed as a contributor. I've sent a more recent-follow up with the revisions and giving him the opportunity to confirm, and haven't heard back yet._
+*   For comments such as https://github.com/diazrenata/birdsize/blob/7469df457989a9016ecc3761b0dd125497a3be51/R/simulate_community.R#L51 if you add 4 hyphens afterwards ----, the comment will appear in the script outline on the right in RStudio IDE (if you use that IDE), helping code navigation. https://blog.r-hub.io/2023/01/26/code-comments-self-explaining-code/#use-comments-for-the-scripts-outline _I've added these to the long community_generate script_.
+*   why have this "trivial" test file? https://github.com/diazrenata/birdsize/blob/main/tests/testthat/test-01_trivial.R _I use this to test that CI is working, but it's not needed now the package is built. Deleted!_
+*   in test code you don't need to write birdsize::: as testthat loads your package code. Example https://github.com/diazrenata/birdsize/blob/7469df457989a9016ecc3761b0dd125497a3be51/tests/testthat/test-02_included_data.R#L13 _These are removed!_
+*   why are some expectations commented out? https://github.com/diazrenata/birdsize/blob/7469df457989a9016ecc3761b0dd125497a3be51/tests/testthat/test-07_simulate_community.R#L34 _These were vestigial, removed!_
+

tests/testthat/test-06_community_generate.R

@@ -27,10 +27,8 @@ test_that("simulation from AOU works", {
   set.seed(22)
   bbs_data_sims <- community_generate(short_bbs_data)
 
-  # expect_true(ncol(bbs_data_sims) == 23)
   expect_true(nrow(bbs_data_sims) == sum(short_bbs_data$speciestotal))
   expect_false(anyNA(bbs_data_sims))
-  # expect_true(all(round(bbs_data_sims$individual_mass[1:5]) == c(5824, 7147, 121, 120, 119)))
 })
 
 test_that("simulation from AOU works with nonstandard abund name", {
@@ -44,10 +42,8 @@ test_that("simulation from AOU works with nonstandard abund name", {
   set.seed(22)
   bbs_data_sims <- community_generate(short_bbs_data, abundance_column_name = "abund")
 
-  # expect_true(ncol(bbs_data_sims) == 23)
   expect_true(nrow(bbs_data_sims) == sum(short_bbs_data$abund))
   expect_false(anyNA(bbs_data_sims))
-  # expect_true(all(round(bbs_data_sims$individual_mass[1:5]) == c(5824, 7147, 121, 120, 119)))
   expect_true(all(bbs_data_sims$sd_method == "AOU lookup"))
 })
 
@@ -63,10 +59,8 @@ test_that("simulation from species name works", {
   set.seed(22)
   bbs_data_sims <- community_generate(short_bbs_data)
 
-  # expect_true(ncol(bbs_data_sims) == 23)
   expect_true(nrow(bbs_data_sims) == sum(short_bbs_data$speciestotal))
   expect_false(anyNA(bbs_data_sims))
-  # expect_true(all(round(bbs_data_sims$individual_mass[1:5]) == c(5824, 7147, 121, 120, 119)))
   expect_true(all(bbs_data_sims$sd_method == "Scientific name lookup"))
 })
 
@@ -85,11 +79,9 @@ test_that("simulation from mean size works", {
   set.seed(22)
   bbs_data_sims <- community_generate(short_bbs_data)
 
-  #  expect_true(ncol(bbs_data_sims) == 23)
   expect_true(nrow(bbs_data_sims) == sum(short_bbs_data$speciestotal))
   expect_true(all(is.na(bbs_data_sims$AOU)))
   expect_true(all(is.na(bbs_data_sims$scientific_name)))
-  # expect_true(all(round(bbs_data_sims$individual_mass[1:5]) == c(5824, 7147, 127, 121, 117)))
   expect_true(all(bbs_data_sims$sd_method == "SD estimated from mean"))
 })
 
@@ -110,11 +102,9 @@ test_that("simulation from mean and sd size works", {
   set.seed(22)
   bbs_data_sims <- community_generate(short_bbs_data)
 
-  # expect_true(ncol(bbs_data_sims) == 23)
   expect_true(nrow(bbs_data_sims) == sum(short_bbs_data$speciestotal))
   expect_true(all(is.na(bbs_data_sims$AOU)))
   expect_true(all(is.na(bbs_data_sims$scientific_name)))
-  # expect_true(all(round(bbs_data_sims$individual_mass[1:5]) == c(5824, 7147, 121, 120, 119)))
   expect_true(all(bbs_data_sims$sd_method == "Mean and SD provided"))
 })