From 88c0deeca57a872204fe94e2988bdeb642b31760 Mon Sep 17 00:00:00 2001 From: Konrad Pagacz Date: Sat, 8 Feb 2025 13:29:09 +0100 Subject: [PATCH] feat(R pkg): added validation for configuration values (#37) * Added validation for configuration values * Failure of the configuration validation will now be mapped to a stop inside the R session instead of showing the raw Rust panic message Closes #36 --- antidotum/tergo/DESCRIPTION | 4 +- antidotum/tergo/R/extendr-wrappers.R | 7 +- antidotum/tergo/R/styling.R | 14 +- antidotum/tergo/man/get_default_config.Rd | 7 +- antidotum/tergo/man/style_text.Rd | 2 +- antidotum/tergo/man/truncate_error.Rd | 12 ++ antidotum/tergo/src/rust/src/lib.rs | 202 ++++++++++++------ antidotum/tergo/tests/testthat.R | 13 ++ .../tergo/tests/testthat/test-style_text.R | 24 +++ 9 files changed, 206 insertions(+), 79 deletions(-) create mode 100644 antidotum/tergo/man/truncate_error.Rd create mode 100644 antidotum/tergo/tests/testthat.R create mode 100644 antidotum/tergo/tests/testthat/test-style_text.R diff --git a/antidotum/tergo/DESCRIPTION b/antidotum/tergo/DESCRIPTION index 9dc78f09..0a5fbde7 100644 --- a/antidotum/tergo/DESCRIPTION +++ b/antidotum/tergo/DESCRIPTION @@ -31,8 +31,10 @@ Suggests: pkgdown, desc, knitr, - rmarkdown + rmarkdown, + testthat (>= 3.0.0) URL: https://rtergo.pagacz.io, https://github.com/kpagacz/tergo BugReports: https://github.com/kpagacz/tergo/issues VignetteBuilder: knitr +Config/testthat/edition: 3 diff --git a/antidotum/tergo/R/extendr-wrappers.R b/antidotum/tergo/R/extendr-wrappers.R index 79cb0de1..97545c2c 100644 --- a/antidotum/tergo/R/extendr-wrappers.R +++ b/antidotum/tergo/R/extendr-wrappers.R @@ -52,10 +52,13 @@ get_config <- function(path) .Call(wrap__get_config, path) #' print(config) #' #' # Make the indent 4 spaces -#' config$indent <- 4 +#' config$indent <- 4L #' #' # Make the maximum line length 80 characters -#' config$line_length <- 80 +#' config$line_length <- 80L +#' +#' # Make the function line breaks double +#' config$function_line_breaks <- "double" get_default_config <- function() .Call(wrap__get_default_config) # nolint end diff --git a/antidotum/tergo/R/styling.R b/antidotum/tergo/R/styling.R index 47675521..85d07f3c 100644 --- a/antidotum/tergo/R/styling.R +++ b/antidotum/tergo/R/styling.R @@ -128,7 +128,7 @@ style_pkg <- function(path = ".", }, error = function(err) { # Print File Path, Red Cross, and Error Message - if (verbose) cat(sprintf("%s %s : %s\n", basename(file), red_cross, err$message)) + if (verbose) cat(sprintf("%s %s : %s\n", basename(file), red_cross, truncate_error(err$message))) } ) } @@ -171,7 +171,7 @@ style_file <- function(file, configuration = list()) { if (formatted[[1]] == "success") { formatted[[2]] } else { - stop("Failed to style the file.") + stop("Failed to style the file. Error: ", truncate_error(formatted[[2]])) } write(x = formatted[[2]], file = file) invisible(NULL) @@ -185,7 +185,7 @@ style_file <- function(file, configuration = list()) { #' #' @inheritParams style #' @param text (`character`) the text to style -#' @return (`character`) the text formatted as R code +#' @return (`character`) The text formatted as R code. #' #' @export #' @examples @@ -202,7 +202,7 @@ style_text <- function(text, configuration = list()) { if (formatted[[1]] == "success") { formatted[[2]] } else { - stop("Failed to style the text.") + stop("Failed to style the text. Error: ", truncate_error(formatted[[2]])) } }, FUN.VALUE = character(1), @@ -210,3 +210,9 @@ style_text <- function(text, configuration = list()) { ) } +#' Truncate the error message +#' +#' @keywords internal +truncate_error <- function(err) { + ifelse(nchar(err) > 80, sprintf("%s...", substr(err, 1, 77)), err) +} diff --git a/antidotum/tergo/man/get_default_config.Rd b/antidotum/tergo/man/get_default_config.Rd index 246b2423..0a94bdd2 100644 --- a/antidotum/tergo/man/get_default_config.Rd +++ b/antidotum/tergo/man/get_default_config.Rd @@ -33,8 +33,11 @@ config <- get_default_config() print(config) # Make the indent 4 spaces -config$indent <- 4 +config$indent <- 4L # Make the maximum line length 80 characters -config$line_length <- 80 +config$line_length <- 80L + +# Make the function line breaks double +config$function_line_breaks <- "double" } diff --git a/antidotum/tergo/man/style_text.Rd b/antidotum/tergo/man/style_text.Rd index ab986327..aa6f9ddf 100644 --- a/antidotum/tergo/man/style_text.Rd +++ b/antidotum/tergo/man/style_text.Rd @@ -12,7 +12,7 @@ style_text(text, configuration = list()) \item{configuration}{(\code{list}) Configuration for formatting. Default \code{list()}.} } \value{ -(\code{character}) the text formatted as R code +(\code{character}) The text formatted as R code. } \description{ Style text diff --git a/antidotum/tergo/man/truncate_error.Rd b/antidotum/tergo/man/truncate_error.Rd new file mode 100644 index 00000000..bfa444f3 --- /dev/null +++ b/antidotum/tergo/man/truncate_error.Rd @@ -0,0 +1,12 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/styling.R +\name{truncate_error} +\alias{truncate_error} +\title{Truncate the error message} +\usage{ +truncate_error(err) +} +\description{ +Truncate the error message +} +\keyword{internal} diff --git a/antidotum/tergo/src/rust/src/lib.rs b/antidotum/tergo/src/rust/src/lib.rs index 284181ec..875a9df9 100644 --- a/antidotum/tergo/src/rust/src/lib.rs +++ b/antidotum/tergo/src/rust/src/lib.rs @@ -1,6 +1,57 @@ use extendr_api::prelude::*; +use std::collections::HashMap; use tergo_lib::{Config, FunctionLineBreaks}; +const ERROR: &str = "error"; +const OK: &str = "success"; + +fn config_to_bool( + field: &str, + configuration: &HashMap<&str, Robj>, + default_value: bool, +) -> std::result::Result { + let config_value = configuration.get(field); + let value: bool; + if let Some(config) = config_value { + if let Some(casted) = config.as_bool() { + value = casted; + } else { + return Err(list!( + ERROR, + format!("{} configuration value must be a boolean.", field) + )); + } + } else { + value = default_value; + } + Ok(value) +} + +fn config_to_integer( + field: &str, + configuration: &HashMap<&str, Robj>, + default_value: i32, +) -> std::result::Result { + let config_value = configuration.get(field); + let value: i32; + if let Some(config) = config_value { + if let Some(casted) = config.as_integer() { + value = casted; + } else { + return Err(list!( + ERROR, + format!( + "{} configuration value must be an integer. Did you forget about L?", + field + ) + )); + } + } else { + value = default_value; + } + Ok(value) +} + /// Format code /// /// @param source_code (`character`) the R code to format @@ -13,71 +64,76 @@ fn format_code(source_code: &str, configuration: extendr_api::List) -> extendr_a let configuration = configuration.into_hashmap(); let default_config = Config::default(); let config = Config::new( - configuration - .get("indent") - .map(|x| x.as_integer().expect("The indent must be an integer")) - .unwrap_or(default_config.indent.0), - configuration - .get("line_length") - .map(|x| x.as_integer().expect("The line_length must be an integer")) - .unwrap_or(default_config.line_length.0), - configuration - .get("embracing_op_no_nl") - .map(|x| { - x.as_bool() - .expect("The embracing_op_no_nl must be a boolean") - }) - .unwrap_or(default_config.embracing_op_no_nl.0), - configuration - .get("allow_nl_after_assignment") - .map(|x| { - x.as_bool() - .expect("The allow_nl_after_assignment must be a boolean") - }) - .unwrap_or(default_config.allow_nl_after_assignment.0), - configuration - .get("space_before_complex_rhs_in_formula") - .map(|x| { - x.as_bool() - .expect("The space_before_complex_rhs_in_formula must be a boolean") - }) - .unwrap_or(default_config.space_before_complex_rhs_in_formula.0), - configuration - .get("strip_suffix_whitespace_in_function_defs") - .map(|x| { - x.as_bool() - .expect("The strip_suffix_whitespace_in_function_defs must be a boolean") - }) - .unwrap_or(default_config.strip_suffix_whitespace_in_function_defs.0), - configuration - .get("function_line_breaks") - .map(|x| { - match x - .as_str() - .expect("The function_line_breaks must be character") - { - "single" => FunctionLineBreaks::Single, - "double" => FunctionLineBreaks::Double, - "hanging" => FunctionLineBreaks::Hanging, - _ => panic!("Unknown function line breaks. Allowed: single, double, hanging."), + match config_to_integer("indent", &configuration, default_config.indent.0) { + Ok(value) => value, + Err(error) => return error, + }, + match config_to_integer("line_length", &configuration, default_config.line_length.0) { + Ok(value) => value, + Err(error) => return error, + }, + match config_to_bool( + "embracing_op_no_nl", + &configuration, + default_config.embracing_op_no_nl.0, + ) { + Ok(value) => value, + Err(error) => return error, + }, + match config_to_bool( + "allow_nl_after_assignment", + &configuration, + default_config.allow_nl_after_assignment.0, + ) { + Ok(value) => value, + Err(error) => return error, + }, + match config_to_bool( + "space_before_complex_rhs_in_formula", + &configuration, + default_config.space_before_complex_rhs_in_formula.0, + ) { + Ok(value) => value, + Err(error) => return error, + }, + match config_to_bool( + "strip_suffix_whitespace_in_function_defs", + &configuration, + default_config.strip_suffix_whitespace_in_function_defs.0, + ) { + Ok(value) => value, + Err(error) => return error, + }, + match configuration.get("function_line_breaks") { + Some(text) => match text.as_str() { + Some("single") => FunctionLineBreaks::Single, + Some("double") => FunctionLineBreaks::Double, + Some("hanging") => FunctionLineBreaks::Hanging, + _ => { + return list!( + ERROR, + "Unknown function line breaks in the configuration value. Allowed: single, double, hanging." + ) } - }) - .unwrap_or(default_config.function_line_breaks), - configuration - .get("insert_newline_in_quote_call") - .map(|x| { - x.as_bool() - .expect("The insert_newline_in_quote_call must be a boolean") - }) - .unwrap_or(default_config.insert_newline_in_quote_call.0), + }, + None => default_config.function_line_breaks, + }, + match config_to_bool( + "insert_newline_in_quote_call", + &configuration, + default_config.insert_newline_in_quote_call.0, + ) { + Ok(value) => value, + Err(error) => return error, + }, ); match tergo_lib::tergo_format(source_code, Some(&config)) { Ok(formatted_code) => { - list!("success", formatted_code) + list!(OK, formatted_code) } Err(error) => { - list!("error", error) + list!(ERROR, error) } } } @@ -139,15 +195,20 @@ fn get_config(path: &str) -> extendr_api::List { /// /// @details /// The configuration values: -/// * indent - the number of spaces to use for indentation. -/// * line_length - the maximum number of characters in a line. -/// * embracing_op_no_nl - whether to allow a newline after an embracing operator. -/// * allow_nl_after_assignment - whether to allow a newline after an assignment operator. -/// * space_before_complex_rhs_in_formula - whether to add a space before a complex right-hand side in a formula. -/// * strip_suffix_whitespace_in_function_defs - whether to strip suffix whitespace in function definitions. -/// * function_line_breaks - the type of line breaks in function definitions when arguments do not -/// fit. Possible values are: hanging, double, single. -/// * insert_newline_in_quote_call - whether to insert a newline in calls to `quote`. +/// * indent (`integer`) - the number of spaces to use for indentation. E.g. 2L, 4L. +/// * line_length (`integer`) - the maximum number of characters in a line. E.g. 80L, 120L. +/// * embracing_op_no_nl (`logical`) - whether to allow a newline after an embracing operator. E.g. +/// TRUE, FALSE. +/// * allow_nl_after_assignment (`logical`) - whether to allow a newline after an assignment operator. +/// E.g. TRUE, FALSE. +/// * space_before_complex_rhs_in_formula (`logical`) - whether to add a space before a complex +/// right-hand side in a formula. E.g. TRUE, FALSE. +/// * strip_suffix_whitespace_in_function_defs (`logical`) - whether to strip suffix +/// whitespace in function definitions. E.g. TRUE, FALSE. +/// * function_line_breaks (`character`) - the type of line breaks in function definitions when arguments do not +/// fit. Possible values are: "hanging", "double", "single". +/// * insert_newline_in_quote_call (`logical`) - whether to insert a newline in calls to `quote`. +/// E.g. TRUE, FALSE. /// /// @return `list` with the default configuration /// @export @@ -156,10 +217,13 @@ fn get_config(path: &str) -> extendr_api::List { /// print(config) /// /// # Make the indent 4 spaces -/// config$indent <- 4 +/// config$indent <- 4L /// /// # Make the maximum line length 80 characters -/// config$line_length <- 80 +/// config$line_length <- 80L +/// +/// # Make the function line breaks double +/// config$function_line_breaks <- "double" #[extendr] fn get_default_config() -> extendr_api::List { let config = Config::default(); diff --git a/antidotum/tergo/tests/testthat.R b/antidotum/tergo/tests/testthat.R new file mode 100644 index 00000000..a0bc0c3b --- /dev/null +++ b/antidotum/tergo/tests/testthat.R @@ -0,0 +1,13 @@ +# This file is part of the standard setup for testthat. +# It is recommended that you do not modify it. +# +# Where should you do additional test configuration? +# Learn more about the roles of various files in: +# * https://r-pkgs.org/testing-design.html#sec-tests-files-overview +# * https://testthat.r-lib.org/articles/special-files.html + +library(testthat) +library(tergo) + +testthat::test_check("tergo") + diff --git a/antidotum/tergo/tests/testthat/test-style_text.R b/antidotum/tergo/tests/testthat/test-style_text.R new file mode 100644 index 00000000..d43ce5b5 --- /dev/null +++ b/antidotum/tergo/tests/testthat/test-style_text.R @@ -0,0 +1,24 @@ +testthat::test_that("style_text validates the configuration", { + testthat::expect_error( + style_text( + "1+1", + configuration = list(line_length = 80), + "Failed to style the text. Error: line_length configuration value must be an integer. Did you forget about L?" + ) + ) + testthat::expect_error( + style_text( + "1+1", + configuration = list(indent = 2), + "Failed to style the text. Error: indent configuration value must be an integer. Did you forget about L?" + ) + ) + testthat::expect_error( + style_text( + "1+1", + configuration = list(embracing_op_no_nl = 2), + "Failed to style the text. Error: embracing_op_no_nl configuration value must be a boolean." + ) + ) +}) +