v2.3.3 #213
v2.3.3 #213
Conversation
syncing dev
CRAN bullshit
WalkthroughThe recent updates involve adding a new GitHub Actions workflow for R-hub testing, upgrading the Changes
Poem
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (43)
Files not reviewed due to errors (1)
Files skipped from review due to trivial changes (14)
Additional comments not posted (40)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Out of diff range and nitpick comments (2)
R/auto_rate.int.R (1)
371-371: Consider renamingby_valtovalidate_byfor clarity.Renaming
by_valtovalidate_bymight enhance readability and make the function's purpose more apparent.tests/testthat/test-select_rate.R (1)
13-13: Ensure that all resources are cleaned up after tests to prevent any side effects on subsequent tests or system state.
| # S, t, P vector inputs should be a single value | ||
| # Should add this to StP.val above | ||
| if (!is.null(S) && length(S) > 1) | ||
| stop("convert_rate: The 'S' input should be a single value. Rates determined in different physical conditions should be analysed and converted separately.") | ||
| if (!is.null(t) && length(t) > 1) | ||
| stop("convert_rate: The 't' input should be a single value. Rates determined in different physical conditions should be analysed and converted separately.") | ||
| if (!is.null(P) && length(P) > 1) | ||
| stop("convert_rate: The 'P' input should be a single value. Rates determined in different physical conditions should be analysed and converted separately.") |
There was a problem hiding this comment.
Ensure consistent error handling for input validation.
- if (!is.null(S) && length(S) > 1)
- stop("convert_rate: The 'S' input should be a single value. Rates determined in different physical conditions should be analysed and converted separately.")
- if (!is.null(t) && length(t) > 1)
- stop("convert_rate: The 't' input should be a single value. Rates determined in different physical conditions should be analysed and converted separately.")
- if (!is.null(P) && length(P) > 1)
- stop("convert_rate: The 'P' input should be a single value. Rates determined in different physical conditions should be analysed and converted separately.")
+ if (!is.null(S) && length(S) != 1)
+ stop("convert_rate: The 'S' input must be a single value. Rates determined in different physical conditions should be analysed and converted separately.")
+ if (!is.null(t) && length(t) != 1)
+ stop("convert_rate: The 't' input must be a single value. Rates determined in different physical conditions should be analysed and converted separately.")
+ if (!is.null(P) && length(P) != 1)
+ stop("convert_rate: The 'P' input must be a single value. Rates determined in different physical conditions should be analysed and converted separately.")The error messages for input validation of S, t, and P should consistently check for exactly one value (length(...) != 1) rather than more than one (length(...) > 1). This ensures that no input is mistakenly passed as an empty vector or NULL, which could bypass the intended checks.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| # S, t, P vector inputs should be a single value | |
| # Should add this to StP.val above | |
| if (!is.null(S) && length(S) > 1) | |
| stop("convert_rate: The 'S' input should be a single value. Rates determined in different physical conditions should be analysed and converted separately.") | |
| if (!is.null(t) && length(t) > 1) | |
| stop("convert_rate: The 't' input should be a single value. Rates determined in different physical conditions should be analysed and converted separately.") | |
| if (!is.null(P) && length(P) > 1) | |
| stop("convert_rate: The 'P' input should be a single value. Rates determined in different physical conditions should be analysed and converted separately.") | |
| # S, t, P vector inputs should be a single value | |
| # Should add this to StP.val above | |
| if (!is.null(S) && length(S) != 1) | |
| stop("convert_rate: The 'S' input must be a single value. Rates determined in different physical conditions should be analysed and converted separately.") | |
| if (!is.null(t) && length(t) != 1) | |
| stop("convert_rate: The 't' input must be a single value. Rates determined in different physical conditions should be analysed and converted separately.") | |
| if (!is.null(P) && length(P) != 1) | |
| stop("convert_rate: The 'P' input must be a single value. Rates determined in different physical conditions should be analysed and converted separately.") |
| stop("import_file: Excel file detected. Excel support has been removed.") | ||
| #raw <- suppressMessages(read_excel(path, n_max = 20)) | ||
| #raw <- as.character(raw) |
There was a problem hiding this comment.
Remove commented-out code related to Excel file handling.
- #raw <- suppressMessages(read_excel(path, n_max = 20))
- #raw <- as.character(raw)This commented-out code is no longer necessary since Excel file support has been removed. Removing it will clean up the codebase and reduce confusion.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| stop("import_file: Excel file detected. Excel support has been removed.") | |
| #raw <- suppressMessages(read_excel(path, n_max = 20)) | |
| #raw <- as.character(raw) | |
| stop("import_file: Excel file detected. Excel support has been removed.") |
| "convert_rate.ft: Input or output units require Salinity input") | ||
| expect_error(convert_rate.ft(-1.4, | ||
| oxy.unit = "mL/L", flowrate.unit = "l/s", | ||
| output.unit = "mg/h/mm2", area = 2, mass = NULL, | ||
| S = 35, t = NULL, P = NULL), | ||
| "convert_DO: Input or output units require Temperature input") | ||
| "convert_rate.ft: Input or output units require Temperature input") | ||
| expect_message(convert_rate.ft(-1.4, | ||
| oxy.unit = "mL/L", flowrate.unit = "l/s", | ||
| output.unit = "mg/h/mm2", area = 2, mass = NULL, | ||
| S = 35, t = 15, P = NULL), | ||
| "convert_DO: Input or output units require Atmospheric Pressure input") | ||
| "convert_rate.ft: Input or output units require Atmospheric Pressure input") | ||
| }) |
There was a problem hiding this comment.
Refine error messages for clarity and consistency.
- "convert_rate.ft: Input or output units require Salinity input"
+ "convert_rate.ft: Salinity input is required for the specified units."
- "convert_rate.ft: Input or output units require Temperature input"
+ "convert_rate.ft: Temperature input is required for the specified units."
- "convert_rate.ft: Input or output units require Atmospheric Pressure input"
+ "convert_rate.ft: Atmospheric Pressure input is required for the specified units."The error messages should clearly state the requirement for input parameters when they are missing, enhancing the user's understanding and the code's maintainability.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| "convert_rate.ft: Input or output units require Salinity input") | |
| expect_error(convert_rate.ft(-1.4, | |
| oxy.unit = "mL/L", flowrate.unit = "l/s", | |
| output.unit = "mg/h/mm2", area = 2, mass = NULL, | |
| S = 35, t = NULL, P = NULL), | |
| "convert_DO: Input or output units require Temperature input") | |
| "convert_rate.ft: Input or output units require Temperature input") | |
| expect_message(convert_rate.ft(-1.4, | |
| oxy.unit = "mL/L", flowrate.unit = "l/s", | |
| output.unit = "mg/h/mm2", area = 2, mass = NULL, | |
| S = 35, t = 15, P = NULL), | |
| "convert_DO: Input or output units require Atmospheric Pressure input") | |
| "convert_rate.ft: Input or output units require Atmospheric Pressure input") | |
| }) | |
| "convert_rate.ft: Salinity input is required for the specified units.") | |
| expect_error(convert_rate.ft(-1.4, | |
| oxy.unit = "mL/L", flowrate.unit = "l/s", | |
| output.unit = "mg/h/mm2", area = 2, mass = NULL, | |
| S = 35, t = NULL, P = NULL), | |
| "convert_rate.ft: Temperature input is required for the specified units.") | |
| expect_message(convert_rate.ft(-1.4, | |
| oxy.unit = "mL/L", flowrate.unit = "l/s", | |
| output.unit = "mg/h/mm2", area = 2, mass = NULL, | |
| S = 35, t = 15, P = NULL), | |
| "convert_rate.ft: Atmospheric Pressure input is required for the specified units.") | |
| }) |
| #### @perryKelpHoldfastMicroclimates2024 | ||
|
|
||
| A publication from Frances Perry's [MSc project](https://januarharianto.github.io/respR/articles/citations.html#perryMicroclimatesHoldfastsBull2023), this looked at thermal microclimates amongst kelp holdfasts, and used `respR` in getting metabolic rates of a gastropod and an amphipod. These microclimates seem to provide some buffering against external warming and may be something of a refuge for invertebrates, which is really interesting. | ||
|
|
||
| #### @jarquin-corroEvaluationCompositionIsotopique2024 | ||
|
|
||
| Santiago Jarquin-Corro's MSc thesis conducted at Université du Québec à Rimouski examined the American eel *Anguilla rostrata*, and looks at the use carbon in otoliths as a proxy for metabolic rate. Respirometry was used to validate the results. Congrats on the MSc! 🥳 | ||
|
|
There was a problem hiding this comment.
Consider enhancing the robustness and efficiency of the citation count extraction code.
- dump <- suppressWarnings(readLines("https://scholar.google.co.uk/scholar?cites=15799904892345725732", encoding = "latin1"))
+ # Use httr to handle HTTP errors and timeouts more gracefully
+ library(httr)
+ response <- GET("https://scholar.google.co.uk/scholar?cites=15799904892345725732")
+ stop_for_status(response)
+ dump <- content(response, "text", encoding = "UTF-8")
- loc <- grep("About [0-9]+ results \\(", dump)
+ # Use a more robust regex pattern and handle cases where the pattern is not found
+ pattern <- "About [0-9]+ results \\("
+ loc <- grep(pattern, dump)
+ if (length(loc) == 0) stop("Citation count pattern not found.")This refactoring uses the httr package for more reliable HTTP requests and improves error handling by checking the HTTP response status and handling cases where the citation count pattern is not found.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| #### @perryKelpHoldfastMicroclimates2024 | |
| A publication from Frances Perry's [MSc project](https://januarharianto.github.io/respR/articles/citations.html#perryMicroclimatesHoldfastsBull2023), this looked at thermal microclimates amongst kelp holdfasts, and used `respR` in getting metabolic rates of a gastropod and an amphipod. These microclimates seem to provide some buffering against external warming and may be something of a refuge for invertebrates, which is really interesting. | |
| #### @jarquin-corroEvaluationCompositionIsotopique2024 | |
| Santiago Jarquin-Corro's MSc thesis conducted at Université du Québec à Rimouski examined the American eel *Anguilla rostrata*, and looks at the use carbon in otoliths as a proxy for metabolic rate. Respirometry was used to validate the results. Congrats on the MSc! 🥳 | |
| #### @perryKelpHoldfastMicroclimates2024 | |
| A publication from Frances Perry's [MSc project](https://januarharianto.github.io/respR/articles/citations.html#perryMicroclimatesHoldfastsBull2023), this looked at thermal microclimates amongst kelp holdfasts, and used `respR` in getting metabolic rates of a gastropod and an amphipod. These microclimates seem to provide some buffering against external warming and may be something of a refuge for invertebrates, which is really interesting. | |
| #### @jarquin-corroEvaluationCompositionIsotopique2024 | |
| Santiago Jarquin-Corro's MSc thesis conducted at Université du Québec à Rimouski examined the American eel *Anguilla rostrata*, and looks at the use carbon in otoliths as a proxy for metabolic rate. Respirometry was used to validate the results. Congrats on the MSc! 🥳 | |
| if (!identical(Sys.getenv("NOT_CRAN"), "true")) return() | ||
| skip_on_cran() | ||
|
|
There was a problem hiding this comment.
Tip
Codebase Verification
The search results indicate that the environment variable NOT_CRAN is used extensively across various test files in the repository. However, there is no direct evidence of documentation explaining the purpose or usage of NOT_CRAN within the repository. The references found are primarily usage instances within test conditions, and a couple of instances where the environment variable is set, but without accompanying explanatory comments or documentation.
Given the absence of explicit documentation in the provided search results, it is reasonable to conclude that the environment variable NOT_CRAN is not documented in the repository. This could potentially lead to confusion or misuse of the environment variable, especially for new contributors or when maintaining the code.
Documentation for NOT_CRAN is missing in the repository
The environment variable NOT_CRAN is used in multiple test scripts to conditionally skip tests, presumably when running on CRAN. However, there is no documentation found in the repository explaining its purpose or usage guidelines. It is recommended to add documentation for this environment variable to improve code clarity and maintainability.
Analysis chain
Ensure the environment variable NOT_CRAN is documented.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the environment variable `NOT_CRAN` is documented in the repository.
# Test: Search for documentation of `NOT_CRAN`. Expect: At least one match.
rg 'NOT_CRAN' --files-with-matches | xargs rg 'NOT_CRAN'
Length of output: 2511
|
|
||
| - Fix for `convert_DO` not accepting vectors for the `P` input. | ||
| - `convert_rate`, `convert_rate.ft`, and `convert_MR` now stop with an error if vectors have been entered for `S`, `t`, or `P` (instead of silently accepting them, but only using the first value in actual conversions). | ||
| - To maintain compatibility with older versions of R, support for Excel files has been removed from `import_file` and as a result `readxl` has been removed as a dependency. The `import_file` function has already been deprecated in a previous update, and we strongly encourage users to move to importing their files using common packages. See [here](https://januarharianto.github.io/respR/articles/importing.html). |
There was a problem hiding this comment.
Address the removal of readxl dependency more explicitly.
- - To maintain compatibility with older versions of R, support for Excel files has been removed from `import_file` and as a result `readxl` has been removed as a dependency.
+ - To maintain compatibility with older versions of R, support for Excel files has been removed from `import_file`. Consequently, the `readxl` package is no longer a dependency.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| - To maintain compatibility with older versions of R, support for Excel files has been removed from `import_file` and as a result `readxl` has been removed as a dependency. The `import_file` function has already been deprecated in a previous update, and we strongly encourage users to move to importing their files using common packages. See [here](https://januarharianto.github.io/respR/articles/importing.html). | |
| - To maintain compatibility with older versions of R, support for Excel files has been removed from `import_file`. Consequently, the `readxl` package is no longer a dependency. The `import_file` function has already been deprecated in a previous update, and we strongly encourage users to move to importing their files using common packages. See [here](https://januarharianto.github.io/respR/articles/importing.html). |
| - Fix for `convert_DO` not accepting vectors for the `P` input. | ||
| - `convert_rate`, `convert_rate.ft`, and `convert_MR` now stop with an error if vectors have been entered for `S`, `t`, or `P` (instead of silently accepting them, but only using the first value in actual conversions). | ||
| - To maintain compatibility with older versions of R, support for Excel files has been removed from `import_file` and as a result `readxl` has been removed as a dependency. The `import_file` function has already been deprecated in a previous update, and we strongly encourage users to move to importing their files using common packages. See [here](https://januarharianto.github.io/respR/articles/importing.html). | ||
| - NEW: Tested with `R` version [4.4.0](https://www.r-bloggers.com/2023/04/whats-new-in-r-4-4-0/) |
There was a problem hiding this comment.
Correct the Markdown link format for R version 4.4.0 to ensure it is clickable.
- - NEW: Tested with `R` version [4.4.0](https://www.r-bloggers.com/2023/04/whats-new-in-r-4-4-0/)
+ - NEW: Tested with `R` version [4.4.0](https://www.r-bloggers.com/2023/04/whats-new-in-r-4-4-0/)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| - NEW: Tested with `R` version [4.4.0](https://www.r-bloggers.com/2023/04/whats-new-in-r-4-4-0/) | |
| - NEW: Tested with `R` version [4.4.0](https://www.r-bloggers.com/2023/04/whats-new-in-r-4-4-0/) |
| if (length(S) > 1 && length(S) != length(x)) | ||
| stop("convert_DO: The 'S' input must be a single value or the same length as the rates to be converted.") | ||
| if (length(t) > 1 && length(t) != length(x)) | ||
| stop("convert_DO: The 't' input must be a single value or the same length as the rates to be converted.") | ||
| if (length(P) > 1 && length(P) != length(x)) | ||
| stop("convert_DO: The 'P' input must be a single value or the same length as the rates to be converted.") |
There was a problem hiding this comment.
Ensure consistent input validation for parameters S, t, and P.
- if (length(S) > 1 && length(S) != length(x))
- stop("convert_DO: The 'S' input must be a single value or the same length as the rates to be converted.")
- if (length(t) > 1 && length(t) != length(x))
- stop("convert_DO: The 't' input must be a single value or the same length as the rates to be converted.")
- if (length(P) > 1 && length(P) != length(x))
- stop("convert_DO: The 'P' input must be a single value or the same length as the rates to be converted.")
+ if (length(S) > 1 && length(S) != length(x)) {
+ stop("convert_DO: The 'S' input must be a single value or the same length as the rates to be converted.", call. = FALSE)
+ }
+ if (length(t) > 1 && length(t) != length(x)) {
+ stop("convert_DO: The 't' input must be a single value or the same length as the rates to be converted.", call. = FALSE)
+ }
+ if (length(P) > 1 && length(P) != length(x)) {
+ stop("convert_DO: The 'P' input must be a single value or the same length as the rates to be converted.", call. = FALSE)
+ }The input validation checks for S, t, and P are crucial for ensuring that the function handles data correctly. However, the error messages should not terminate the function call stack (call. = FALSE) to provide more context in debugging. Also, ensure that the checks are wrapped in braces for better readability and to prevent potential errors in future modifications.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if (length(S) > 1 && length(S) != length(x)) | |
| stop("convert_DO: The 'S' input must be a single value or the same length as the rates to be converted.") | |
| if (length(t) > 1 && length(t) != length(x)) | |
| stop("convert_DO: The 't' input must be a single value or the same length as the rates to be converted.") | |
| if (length(P) > 1 && length(P) != length(x)) | |
| stop("convert_DO: The 'P' input must be a single value or the same length as the rates to be converted.") | |
| if (length(S) > 1 && length(S) != length(x)) { | |
| stop("convert_DO: The 'S' input must be a single value or the same length as the rates to be converted.", call. = FALSE) | |
| } | |
| if (length(t) > 1 && length(t) != length(x)) { | |
| stop("convert_DO: The 't' input must be a single value or the same length as the rates to be converted.", call. = FALSE) | |
| } | |
| if (length(P) > 1 && length(P) != length(x)) { | |
| stop("convert_DO: The 'P' input must be a single value or the same length as the rates to be converted.", call. = FALSE) | |
| } |
| skip_on_cran() | ||
|
|
There was a problem hiding this comment.
Consider adding a message indicating that tests are skipped when NOT_CRAN is "true".
+ message("Skipping tests on CRAN.")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| message("Skipping tests on CRAN.") |
The setup for test objects is quite complex and could benefit from further modularization or comments explaining each step for maintainability.
Add more comprehensive edge case tests for the select_rate function to ensure all potential input variations are handled correctly.
Would you like me to help generate additional test cases?
| @@ -330,7 +330,7 @@ calc_rate.int <- function(x, | |||
|
|
|||
| ## verify by input | |||
| ## - this is also done in calc_rate but we want to disallow "oxygen" | |||
| by <- by.val(by, req = TRUE, default = "row", | |||
| by <- by_val(by, req = TRUE, default = "row", | |||
There was a problem hiding this comment.
Consider handling the case where by is "oxygen", which is explicitly unsupported.
- by <- by_val(by, req = TRUE, default = "row",
- which = c("t", "r"),
- msg = "calc_rate.int")
+ by <- by_val(by, req = TRUE, default = "row",
+ which = c("t", "r", "oxygen"),
+ msg = "calc_rate.int: 'oxygen' is not supported.")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| by <- by_val(by, req = TRUE, default = "row", | |
| by <- by_val(by, req = TRUE, default = "row", | |
| which = c("t", "r", "oxygen"), | |
| msg = "calc_rate.int: 'oxygen' is not supported.") |
Clarify the message regarding the selection of the first two columns to include information about how to specify columns if needed.
- message("calc_rate.int: Multi-column dataset detected in input. Selecting first two columns by default.\n If these are not the intended data, inspect() or subset the data frame columns appropriately before running calc_rate.int()")
+ message("calc_rate.int: Multi-column dataset detected in input. Selecting first two columns by default. If these are not the intended data, use inspect() with the appropriate columns or subset the data frame columns before running calc_rate.int().")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| by <- by_val(by, req = TRUE, default = "row", | |
| by <- by_val(by, req = TRUE, default = "row", | |
| message("calc_rate.int: Multi-column dataset detected in input. Selecting first two columns by default. If these are not the intended data, use inspect() with the appropriate columns or subset the data frame columns before running calc_rate.int().") |
Ensure consistent handling of starts, wait, and measure when they are provided as vectors.
- if(!is.null(measure) && length(measure) > 1 && length(measure) != length(starts))
- stop("calc_rate.int: For a vector input 'measure' should be the same length as 'starts'.")
+ if(!is.null(measure) && length(measure) != length(starts))
+ stop("calc_rate.int: For a vector input, 'measure' should be the same length as 'starts'.")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| by <- by_val(by, req = TRUE, default = "row", | |
| by <- by_val(by, req = TRUE, default = "row", |
Summary by CodeRabbit
New Features
calc_pcrit()to enhance analysis capabilities in therespRpackage.import_file()function, excluding support for Excel files.Bug Fixes
convert_DOandconvert_rate.by.valtoby_valfor improved input validation.Documentation
respRpackage description.Tests
Chores
cran-comments.mdto address a test failure.