From 1e564eac853d5165041c30f1069f0a9cde36ab11 Mon Sep 17 00:00:00 2001 From: Sam Schiano <125507018+Schiano-NOAA@users.noreply.github.com> Date: Tue, 1 Oct 2024 14:47:47 -0400 Subject: [PATCH 01/10] fix(README): fix example so parameters are added into the YAML The example had parameters = FALSE, but the default child documents contained a reference to a parameter as an example for use in-text. This will be fixed by setting parameters = TRUE (default), but also by removing the line in the file. Future development will redesign this example and provide an example folder where users can see the product of create_template and rendering of the doc --- README.md | 1 - 1 file changed, 1 deletion(-) diff --git a/README.md b/README.md index 5b1b2fbd..7cc7ada7 100644 --- a/README.md +++ b/README.md @@ -42,7 +42,6 @@ asar::create_template( year = 2010, author = c("John Snow", "Danny Phantom", "Patrick Star"), include_affiliation = TRUE, - parameters = FALSE, resdir = "C:/Users/Documents/Example_Files", model_results = "Report.sso", model = "SS3" From 5f74433dda7dd7d5255abb13a801f053945d8d1a Mon Sep 17 00:00:00 2001 From: Schiano-NOAA Date: Tue, 1 Oct 2024 16:56:16 -0400 Subject: [PATCH 02/10] feat(skeleton): add section tagging to default skeleton --- inst/templates/skeleton/acknowledgments.qmd | 2 +- inst/templates/skeleton/appendix.qmd | 2 +- inst/templates/skeleton/data.qmd | 2 +- inst/templates/skeleton/discussion.qmd | 2 +- inst/templates/skeleton/executive_summary.qmd | 6 +++--- inst/templates/skeleton/introduction.qmd | 10 +++++----- inst/templates/skeleton/modeling_approach.qmd | 14 +++++++------- inst/templates/skeleton/projections.qmd | 2 +- inst/templates/skeleton/references.qmd | 8 +------- inst/templates/skeleton/results.qmd | 12 ++++++------ 10 files changed, 27 insertions(+), 33 deletions(-) diff --git a/inst/templates/skeleton/acknowledgments.qmd b/inst/templates/skeleton/acknowledgments.qmd index 8fa8c479..c2f08519 100644 --- a/inst/templates/skeleton/acknowledgments.qmd +++ b/inst/templates/skeleton/acknowledgments.qmd @@ -1 +1 @@ -## Acknowledgements +## Acknowledgements {#sec-acknowledgements} diff --git a/inst/templates/skeleton/appendix.qmd b/inst/templates/skeleton/appendix.qmd index c4ab246d..db68a12e 100644 --- a/inst/templates/skeleton/appendix.qmd +++ b/inst/templates/skeleton/appendix.qmd @@ -1 +1 @@ -## Appendices +## Appendices {#sec-appendix} diff --git a/inst/templates/skeleton/data.qmd b/inst/templates/skeleton/data.qmd index 194f2ae4..208bd248 100644 --- a/inst/templates/skeleton/data.qmd +++ b/inst/templates/skeleton/data.qmd @@ -1,4 +1,4 @@ -## Data +## Data {#sec-data} -### Assessment Methods +### Assessment Methods {#sec-assessment-methods} -### Stock Status +### Stock Status {#sec-stock-status} -### Stock Structure and ID +### Stock Structure and ID {#sec-stock-id} -### Fishery Descriptions +### Fishery Descriptions {#sec-fishery-desscriptions} -### Ecosystem Considerations or climate indicators +### Ecosystem Considerations or climate indicators {#sec-eco-indicators} -### Configuration +### Configuration {#sec-configuration} -### Sensitivity Analyses +### Sensitivity Analyses {#sec-model-sensitivity-analyses} -### Management Benchmarks +### Management Benchmarks {#sec-management-benchmarks} -### Diagnostics +### Diagnostics {#sec-diagnostics} -### Projection Scenarios +### Projection Scenarios {#sec-projection-scenarios} -### Recruitment Estimates and Deviations +### Recruitment Estimates and Deviations {#recruitment-estimates} -### Model Fits +### Model Fits {#sec-model-fits} -### Model Diagnostics +### Model Diagnostics {#sec-model-diagnostics} -### Sensitivity Analysis +### Sensitivity Analysis {#sec-res-sensitivity-analyses} + +# What is the feature? +* + +# How have you implemented the solution? +* + +# Does the PR impact any other area of the project, maybe another repo? +* diff --git a/.github/workflows/pr-checklist.yml b/.github/workflows/pr-checklist.yml new file mode 100644 index 00000000..6f258dfd --- /dev/null +++ b/.github/workflows/pr-checklist.yml @@ -0,0 +1,44 @@ +# This workflow automatically adds a comment containing a reviewer checklist +# when a new pull request is opened. +name: Add a comment with reviewer checklist when PR opened +on: + pull_request: + types: [opened] +jobs: + pr-checklist: + runs-on: ubuntu-latest + name: pr-checklist + steps: + - name: Checkout + uses: actions/checkout@v3 + - name: 'Comment PR' + uses: actions/github-script@0.3.0 + if: github.event_name == 'pull_request' + with: + github-token: ${{ secrets.GITHUB_TOKEN }} + script: | + var msg = `# Instructions for code reviewer + + Hello reviewer, thanks for taking the time to review this PR! + + - Please use this checklist during your review, checking off items that you have verified are complete! + - For PRs that don't make changes to code (e.g., changes to README.md or Github actions workflows), feel free to skip over items on the checklist that are not relevant. Remember it is still important to do a thorough review. + - Then, comment on the pull request with your review indicating where you have questions or changes need to be made before merging. + - Remember to review **every line of code** you’ve been asked to review, look at the context, make sure you’re improving code health, and compliment developers on good things that they do. + - PR reviews are a great way to learn, so feel free to share your tips and tricks. However, for optional changes (i.e., not required for merging), please include \`nit:\` (for nitpicking) before making the suggestion. For example, \`nit:\` I prefer using a \`data.frame()\` instead of a \`matrix\` because... + - Engage with the developer when they respond to comments and check off additional boxes as they become complete so the PR can be merged in when all the tasks are fulfilled. Make it clear when this has been reached by commenting on the PR with something like \`This PR is now ready to be merged, no changes needed\`. + + ## Checklist + + - [ ] The PR is requested to be merged into the appropriate branch (typically main) + - [ ] The code is well-designed. + - [ ] The functionality is good for the users of the code. + - [ ] Any User Interface changes are sensible and look good. + - [ ] The code isn’t more complex than it needs to be. + - [ ] Code coverage remains high, indicating the new code is tested. + - [ ] The developer used clear names for everything. + - [ ] Comments are clear and useful, and mostly explain why instead of what. + - [ ] Code is appropriately documented (doxygen and roxygen). + ` + const { issue: { number: issue_number }, repo: { owner, repo } } = context; + github.issues.createComment({ issue_number, owner, repo, body: msg }); From f92cbe959a8dacad2bb9fab34928ea71b635125d Mon Sep 17 00:00:00 2001 From: Bai-Li-NOAA Date: Mon, 7 Oct 2024 15:50:37 +0000 Subject: [PATCH 10/10] style and docs: run devtools::document() and styler::style_pkg() --- DESCRIPTION | 2 +- R/add_theme.R | 5 ++- R/create_template.R | 32 +++++++++++-------- .../fixtures/prepare_ss3_output_files.R | 6 ++-- tests/testthat/test-add_theme.R | 28 ++++++++-------- tests/testthat/test-convert_output.R | 5 +-- 6 files changed, 39 insertions(+), 39 deletions(-) diff --git a/DESCRIPTION b/DESCRIPTION index 7e24106e..9958ccf4 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -34,7 +34,7 @@ Suggests: parallel, r4ss, rmarkdown, - snowfall, + snowfall, testthat (>= 3.0.0) VignetteBuilder: knitr diff --git a/R/add_theme.R b/R/add_theme.R index b32627c1..7ea07dfa 100644 --- a/R/add_theme.R +++ b/R/add_theme.R @@ -32,7 +32,7 @@ add_theme <- function(x) { theme_obj <- x # gt object } else if ("kableExtra" %in% class(x) || - (length(class(x)) > 1 && class(x)[2] == "knitr_kable")) { + (length(class(x)) > 1 && class(x)[2] == "knitr_kable")) { theme_obj <- x } else if ("gg" %in% class(x) || "ggplot" %in% class(x)) { theme_obj <- x + @@ -55,8 +55,7 @@ add_theme <- function(x) { # move_legend <- theme_obj + # ggplot2::theme() # } - - } else { + } else { message("NOAA formatting cannot be applied to this object.") } diff --git a/R/create_template.R b/R/create_template.R index 6bee258f..eea7e802 100644 --- a/R/create_template.R +++ b/R/create_template.R @@ -469,23 +469,27 @@ create_template <- function( # Convert output file if TRUE if (convert_output) { print("__________Converting output file__________") - if(tolower(model) == "bam" & is.null(fleet_names)) { + if (tolower(model) == "bam" & is.null(fleet_names)) { warning("Fleet names not defined.") } else if (tolower(model) == "bam") { - convert_output(output_file = model_results, - outdir = resdir, - file_save = TRUE, - model = model, - fleet_names = fleet_names, - savedir = subdir, - save_name = paste(sub(" ", "_", species), "_std_res_", year, sep = "")) + convert_output( + output_file = model_results, + outdir = resdir, + file_save = TRUE, + model = model, + fleet_names = fleet_names, + savedir = subdir, + save_name = paste(sub(" ", "_", species), "_std_res_", year, sep = "") + ) } else { - convert_output(output_file = model_results, - outdir = resdir, - file_save = TRUE, - model = model, - savedir = subdir, - save_name = paste(species, "_std_res_", year, sep = "")) + convert_output( + output_file = model_results, + outdir = resdir, + file_save = TRUE, + model = model, + savedir = subdir, + save_name = paste(species, "_std_res_", year, sep = "") + ) } } diff --git a/tests/testthat/fixtures/prepare_ss3_output_files.R b/tests/testthat/fixtures/prepare_ss3_output_files.R index 49640b09..5980c359 100644 --- a/tests/testthat/fixtures/prepare_ss3_output_files.R +++ b/tests/testthat/fixtures/prepare_ss3_output_files.R @@ -9,7 +9,7 @@ ss3_exe <- r4ss::get_ss3_exe( ) # Function to run a specific SS3 model -run_ss3_models <- function(model_id, model_dir, exe_dir){ +run_ss3_models <- function(model_id, model_dir, exe_dir) { # Define the working directory for the model working_dir <- model_dir[model_id] @@ -23,8 +23,8 @@ run_ss3_models <- function(model_id, model_dir, exe_dir){ # List all the model directories to be processed all_models <- list.dirs( file.path(test_path("fixtures", "ss3_models"), "models"), - full.names = TRUE, - recursive = FALSE + full.names = TRUE, + recursive = FALSE ) # Detect the number of available CPU cores, leaving one free diff --git a/tests/testthat/test-add_theme.R b/tests/testthat/test-add_theme.R index 83d16339..b84508e3 100644 --- a/tests/testthat/test-add_theme.R +++ b/tests/testthat/test-add_theme.R @@ -1,5 +1,4 @@ test_that("add_theme applies NOAA formatting correctly", { - # Test with a ggplot object ggplot_obj <- ggplot2::ggplot(data = cars, ggplot2::aes(x = speed, y = dist)) + ggplot2::geom_point() @@ -32,12 +31,11 @@ test_that("add_theme applies NOAA formatting correctly", { unsupported_obj <- data.frame(a = 1, b = 2) expect_message(add_theme(unsupported_obj), "NOAA formatting cannot be applied to this object.") - unsupported_obj2 <- list(1,3,4) + unsupported_obj2 <- list(1, 3, 4) expect_message(add_theme(unsupported_obj2), "NOAA formatting cannot be applied to this object.") }) test_that("nmfspalette returns correct scales", { - # Test 1: Ensure nmfspalette integrates with ggplot color scales ggplot_obj <- ggplot2::ggplot(data = cars, ggplot2::aes(x = speed, y = dist, color = speed)) + ggplot2::geom_point() + @@ -55,7 +53,6 @@ test_that("nmfspalette returns correct scales", { # Check if the correct fill scale is applied expect_true("ScaleDiscrete" %in% class(ggplot_fill_obj$scales$scales[[1]])) expect_true(ggplot_fill_obj$scales$scales[[1]]$aesthetics == "fill") - }) test_that("nmfspalette scales are applied to ggplot object", { @@ -72,14 +69,19 @@ test_that("nmfspalette scales are applied to ggplot object", { expect_true(grepl("nmfs", deparse(color_scale$call))) # Check if nmfspalette scale is used # Ensure the color scale is the default "oceans" nmfspalette scale - test_plot <- ggplot2::ggplot(Orange, - ggplot2::aes( - x = age, - y = circumference, - color = Tree)) + - ggplot2::geom_smooth(se = F, - method = "loess", - formula = "y ~ x") + test_plot <- ggplot2::ggplot( + Orange, + ggplot2::aes( + x = age, + y = circumference, + color = Tree + ) + ) + + ggplot2::geom_smooth( + se = F, + method = "loess", + formula = "y ~ x" + ) ## use default color palette plot_default <- test_plot + @@ -98,7 +100,6 @@ test_that("nmfspalette scales are applied to ggplot object", { ## test if plots' colors are identical expect_equal(colors_default, colors_oceans) # Check if the colors match - }) # Test for both color and fill scales @@ -118,4 +119,3 @@ test_that("nmfspalette scales apply both color and fill scales", { fill_scale <- scales[[2]] expect_true(grepl("nmfs", deparse(fill_scale$call))) # Fill scale from nmfspalette }) - diff --git a/tests/testthat/test-convert_output.R b/tests/testthat/test-convert_output.R index 617f7611..df505474 100644 --- a/tests/testthat/test-convert_output.R +++ b/tests/testthat/test-convert_output.R @@ -1,5 +1,4 @@ test_that("convert_output works for SS3", { - # Get a list of directories containing SS3 models all_models <- list.dirs( file.path(test_path("fixtures", "ss3_models"), "models"), @@ -8,8 +7,7 @@ test_that("convert_output works for SS3", { ) # Loop through each model directory and check convert_output function - for (i in seq_along(all_models)){ - + for (i in seq_along(all_models)) { # Ensure no errors occur while converting SS3 output expect_no_error(result <- convert_output( output_file = "Report.sso", @@ -40,5 +38,4 @@ test_that("convert_output works for SS3", { # Ensure the CSV file was created successfully expect_true(file.exists(file.path(tempdir(), "std_model_output.csv"))) - })