Further implementation of FR #71. Many TODOs done now

aryoda · Aug 1, 2022 · 1e96da6 · 1e96da6
1 parent 4bd6eb0
commit 1e96da6
Show file tree

Hide file tree

Showing 26 changed files with 367 additions and 140 deletions.
diff --git a/DESCRIPTION b/DESCRIPTION
@@ -22,6 +22,8 @@ RoxygenNote: 7.1.1
 Suggests:
     futile.logger,
     lgr,
+    logger,
+    logging,
     testthat,
     knitr,
     rmarkdown,

diff --git a/NAMESPACE b/NAMESPACE
@@ -2,6 +2,7 @@
 
 export(Severity.Levels)
 export(build.log.output)
+export(config.add.row)
 export(config.create)
 export(config.load)
 export(config.save)

diff --git a/R/build_log_output.R b/R/build_log_output.R
@@ -65,8 +65,6 @@ build.log.output <- function(log.results,
                              include.timestamp          = FALSE,
                              use.platform.newline       = FALSE) {
 
-  # TODO Add arguments for incl.timestamp + incl.severity later (redundant output if a logging framework is used!)
-
   stopifnot("data.frame" %in% class(log.results))
 
 

diff --git a/R/config_add_row.R b/R/config_add_row.R
@@ -1 +1,46 @@
-# TODO Add a new (validated) row to the config element
+
+#' Add a new entry (row) to an existing configuration
+#'
+#' You could also add multiple rows in one call by passing vectors of the same length for each argument
+#' (except for the \code{config} argument of course).
+#'
+#' The config passed in as argument is NOT modified, you have to reassign the return value for this (see examples).
+#'
+#' For a detailed description of the argument semantics see the related function \code{\link{config.create}}.
+#'
+#' @param config                     an existing configuration
+#' @param cond.class                 the class name of the condition as character (eg. "error")
+#' @param silent                     \code{\link{logical}}: \code{TRUE} = do not propagate the condition to other registered handlers (= "muffle" in R speak).
+#' @param write.to.log               \code{\link{logical}}: \code{TRUE} = Call the logging function for the caught condition
+#' @param log.as.severity            Severity level for the \code{cond.class} (use the constants from \code{\link{Severity.Levels}} or the equivalent character strings)
+#' @param include.full.call.stack    \code{\link{logical}}: Shall the full call stack be included in the log output?
+#' @param include.compact.call.stack \code{\link{logical}}: Shall the compact call stack (including only calls with source code references)
+#'                                   be included in the log output?
+#'
+#' @return The new config that includes the new row(s)
+#' @export
+#'
+#' @examples
+#' config <- config.create()
+#' config <- config.add.row(config, "myInfo")
+#' config <- config.add.row(config, "myError", FALSE, TRUE, Severity.Levels$FATAL, TRUE, TRUE)
+#' options("tryCatchLog.global.config" = config)
+config.add.row <- function(   config
+                            , cond.class
+                            , silent                      = FALSE
+                            , write.to.log                = TRUE
+                            , log.as.severity             = tryCatchLog::Severity.Levels$INFO
+                            , include.full.call.stack     = FALSE
+                            , include.compact.call.stack  = TRUE)
+{
+
+  config.validate(config, throw.error.with.findings = TRUE)
+
+  new.config.row <- config.create(cond.class, silent, write.to.log, log.as.severity, include.full.call.stack, include.compact.call.stack)
+
+  new.config <- rbind(config, new.config.row, stringsAsFactors = FALSE)
+
+  config.validate(config, throw.error.with.findings = TRUE)
+
+  return(new.config)
+}
diff --git a/R/config_create.R b/R/config_create.R
@@ -23,8 +23,9 @@
 #' The default values create a simple configuration that can be used as example
 #' or to be extended (eg. by saving it as CSV file that can be edited then).
 #'
-#' TODO Clarify who wins in case of conflicts: The function call argument or the configuration?
-#' TODO Explain what happens if a condition inherits from multiple configuration row classes (who wins?)!
+#' TODO Document behaviour if tryCatchLog() is called with logged.conditions = c("myCondition") etc... Who wins?
+#' TODO Document who wins in case of conflicts: The function call argument or the configuration?
+#' TODO Document what happens if a condition inherits from multiple configuration row classes (who wins?)!
 #'      -> if a condition inherits from multiple classes the class attribute looks like this:
 #'           > x <- simpleWarning("a")
 #'           > class(x)
@@ -40,17 +41,14 @@
 #'                                                          The logging framework may still decide to suppress the log output eg. by severity level filtering.
 #'                                   Note that some condition classes cannot be muffled (no support for that in base R), eg. \code{error}
 #'                                   and this setting will be ignored then.
-#' @param write.to.log               \code{\link{logical}}: \code{TRUE} = Call the logging functon for the caught condition)
-#' TODO: Document behaviour if tryCatchLog() is called with logged.conditions = c("myCondition") etc... Who wins?
+#' @param write.to.log               \code{\link{logical}}: \code{TRUE} = Call the logging function for the caught condition
 #' @param log.as.severity            Severity level for the \code{cond.class} (use the constants from \code{\link{Severity.Levels}} or the equivalent character strings)
-#' @param include.full.call.stack    Flag of type \code{\link{logical}}:
-#'                                   Shall the full call stack be included in the log output? Since the full
+#' @param include.full.call.stack    \code{\link{logical}}: Shall the full call stack be included in the log output? Since the full
 #'                                   call stack may be very long and the compact call stack has enough details
 #'                                   normally the full call stack can be omitted by passing \code{FALSE}.
 #'                                   The default value can be changed globally by setting the option \code{tryCatchLog.include.full.call.stack}.
 #'                                   The full call stack can always be found via \code{\link{last.tryCatchLog.result}}.
-#' @param include.compact.call.stack Flag of type \code{\link{logical}}:
-#'                                   Shall the compact call stack (including only calls with source code references)
+#' @param include.compact.call.stack \code{\link{logical}}: Shall the compact call stack (including only calls with source code references)
 #'                                   be included in the log output? Note: If you omit both the full and compact
 #'                                   call stacks the message text will be output without call stacks.
 #'                                   The default value can be changed globally by setting the option \code{tryCatchLog.include.compact.call.stack}.
@@ -79,20 +77,20 @@ config.create <- function(  cond.class                  = c("error", "warning",
 {
 
 
-  # TODO Check preconditions (length and data type, unique cond.class names...)
+  # No preconditions check (like length and data type) required - config.validate() does this later!
 
 
 
-  config <- data.frame(cond.class                 = cond.class,
+  config <- data.frame(# data.frame columns go here --------------------------------------------------------------------------
+                       cond.class                 = cond.class,
                        silent                     = silent, # = muffled = logged only (but not propagated to other registered handlers)!
-                       # see rlang::cnd_muffle()
                        write.to.log               = write.to.log,
                        log.as.severity            = log.as.severity, # these values may be logging-framework-specific (= not good since changing the logging framework requires config changes). Better approach, eg. mapping?
                        include.full.call.stack    = include.full.call.stack,
                        include.compact.call.stack = include.compact.call.stack
                        # -----------------------------------------------------------------------------------------------------
-                       # , rethrow.as = NA              # TODO "reserved" for future extension: Rethrow the condition as another condition class
-                       # , number.of.retires = 0        # TODO "reserved" for future extensions: Number of retries to execute an expression before the condition is really "processed"
+                       # , rethrow.as = NA        # possible future extension: Rethrow the condition as another condition class (issue #30)
+                       # , number.of.retries = 0  # possible future extension: Number of retries to execute an expression before the condition is really "processed" (issue #9)
                        #
                        # data.frame() arguments go here ----------------------------------------------------------------------
                        , row.names = NULL
@@ -105,7 +103,7 @@ config.create <- function(  cond.class                  = c("error", "warning",
 
 
 
-  stopifnot(config.validate(config)$status == TRUE)  # if this happens you have found an internal error ;-)
+  config.validate(config, throw.error.with.findings = TRUE)  # if this happens you have found an internal error ;-)
 
   return (config)
 

diff --git a/R/config_load_save.R b/R/config_load_save.R
@@ -17,7 +17,7 @@
 #' config.save(config.create())
 config.save <- function(config, file.name = "tryCatchLog_config.csv") {
 
-  stopifnot(config.validate(config)$status == TRUE)
+  config.validate(config, throw.error.with.findings = TRUE)
 
 
 
@@ -45,15 +45,12 @@ config.load <- function(file.name = "tryCatchLog_config.csv") {
 
   config <- utils::read.csv2(file.name, stringsAsFactors = FALSE)
 
-  # TODO validate config (maybe reuse precond checks from config.create)
-  #      I would also add a "do.fast.validation.only" argument for a quick partial check from within
-  #      tryCatchLog() where runtime is critical...
-
   # add a class marker to recognize it easier in other functions as valid config
-  # TODO The class name should be an internal "global" constant
   class(config) <- append(.CONFIG.CLASS.NAME, class(config))
 
-  stopifnot(config.validate(config)$status == TRUE)
+
+
+  config.validate(config, throw.error.with.findings = TRUE)
 
   return (config)
 

diff --git a/R/config_validate.R b/R/config_validate.R
@@ -2,15 +2,21 @@ is.config <- function(config) {
 
   res <- list(status = TRUE, findings = "")
 
+  # Design decision:
+  # Should we allow NULL as valid configuration (= no configuration)?
+  # Eg. in config.add.row this would allow to start a new configuration by adding a row to NULL as config
+  # (but why then do not directly call config.create() instead?
+  # -> No, this is counter-intuitive
+
   if (!is.data.frame(config)) {
-    # Also catches NA and NULL as config
-    res$findings <- "Error: The config is not a data.frame!\n"
+    # Also matches NA and NULL as (invalid) config
+    res$findings <- "The config is not a data.frame!\n"
     res$status   <- FALSE
     return(res)
   }
 
-  if (!inherits(config, tryCatchLog:::.CONFIG.CLASS.NAME)) {
-    res$findings <- paste0("Error: The config does not inherit from the '",tryCatchLog:::.CONFIG.CLASS.NAME , "' class but '", class(config), "'!\n")
+  if (!inherits(config, .CONFIG.CLASS.NAME)) {
+    res$findings <- paste0("The config does not inherit from the '", .CONFIG.CLASS.NAME , "' class but only from '", class(config), "'!\n")
     res$status   <- FALSE
     return(res)
   }
@@ -19,8 +25,8 @@ is.config <- function(config) {
 }
 
 
-
-config.validate <- function(config) {
+# Implement throw.error to throw an error with all findings at the end (for reuse purposes - otherwise duplicated at the caller side!)
+config.validate <- function(config, throw.error.with.findings = TRUE) {
 
   # res <- list(status = TRUE, findings = "")
   res <- is.config(config)
@@ -31,31 +37,38 @@ config.validate <- function(config) {
 
   expected_col_names <- c("cond.class", "silent", "write.to.log", "log.as.severity", "include.full.call.stack", "include.compact.call.stack")
   if (any(colnames(config) != expected_col_names)) {
-    res$findings <- paste0(res$findings, "Error: The config does not have the expected column names and order!\n")
+    res$findings <- paste0(res$findings, "The config does not have the expected column names and order!\n")
   }
 
   expected_col_types <- c("character", "logical", "logical", "character", "logical", "logical")
   actual_col_types   <- sapply(config, class)
   if (any(actual_col_types != expected_col_types)) {
-    res$findings <- paste0(res$findings, "Error: The config does not have the expected column classes for all columns!\n")
+    res$findings <- paste0(res$findings, "The config does not have the expected column classes for all columns!\n")
   }
 
   if (anyNA(config)) {
-    res$findings <- paste0(res$findings, "Error: The config has empty cells (NA value)!\n")
+    res$findings <- paste0(res$findings, "The config has empty cells (NA value)!\n")
   }
 
-  if (!all(config$log.as.severity %in% tryCatchLog::Severity.Levels)) {
-    res$findings <- paste0(res$findings, "Error: There are invalid severity level strings in the column 'log.as.severity' (compare to 'tryCatchLog::Severity.Levels')!\n")
+  if ("log.as.severity" %in% colnames(config) && !all(config$log.as.severity %in% tryCatchLog::Severity.Levels)) {
+    res$findings <- paste0(res$findings, "There are invalid severity level strings in the column 'log.as.severity' (compare to 'tryCatchLog::Severity.Levels')!\n")
   }
 
-  # TODO:
-  # - Warn as "unsupported" if "silent" is TRUE for other "warning" and "error"
-  #   -> This is not perfect since any condition class in the config may inherit from error or warning
-  #      so silencing would still work (but there is no reliable ways to find this out by just parsing the configuration
-  #      since it does not contain the class hierarchy!).
+  #  Warn as "unsupported" if "silent" is TRUE for other conditions than "warning" and "error" is NOT possible here
+  #  since we need the caught condition class hierarchy (may inherit from "warning" or "error"
+  #  so that silencing would still work).
+  #  -> There is no reliable way to find this out by just parsing the configuration since it does not contain any class hierarchy!
+
+  if ("cond.class" %in% colnames(config) && any(duplicated(config$cond.class))) {
+    res$findings <- paste0(res$findings, "There are duplicated condition class names in cond.class (undefined semantics)!\n")
+  }
 
   res$status = (res$findings == "")
 
+  if (res$status == FALSE && throw.error.with.findings == TRUE) {
+    stop(paste("Invalid tryCatchLog configuration:\n", res$findings))
+  }
+
   return(res)
 
 }
diff --git a/R/constants.R b/R/constants.R
@@ -17,9 +17,9 @@
 
 
 
-#' Log (severity) levels
+#' List of severity levels for logging output
 #'
-#' These logging levels are supported to characterize the severity of an event in the log output.
+#' Defines the levels that are supported to characterize the severity of an event in the log output.
 #'
 #' The levels comply with the standard Apache log4j log level names (except \code{ALL} to log all events
 #' which is the same as \code{FATAL}).
@@ -35,13 +35,12 @@
 #'   \item \code{INFO}  Indicates an event for informational purposes
 #'   \item \code{DEBUG} A debugging event
 #'   \item \code{TRACE} A fine-grained debug message to capture the execution flow through the application
-#'   \item \code{}
 #' }
 #'
 #' @note In contrast to log4j log levels the log levels of this package do just use names
-#'       but no internal integer log value to order the log levels by severity
+#'       but NO internal integer log value to order the log levels by severity
 #'       since filtering the log output by severity is the responsibility of the used logging framework
-#'       (not the \code{tryCatchLog} package).
+#'       (not the \code{tryCatchLog} package) so we don't need it here.
 #'
 #' @references \url{https://logging.apache.org/log4j/2.0/log4j-api/apidocs/org/apache/logging/log4j/Level.html},
 #'             \url{https://logging.apache.org/log4j/2.x/manual/customloglevels.html}
@@ -53,11 +52,13 @@
 #'
 #' @export
 Severity.Levels <- list(
-  # TODO Decide on best name: Severity.Levels, Log.Levels, Log.Severity.Levels, singular or plural...?
+  # Alternative name candidates were: Log.Levels, Log.Severity.Levels
+  # But since "severity" was already used as column name in the data.frame returned by last.tryCatchLog.result()
+  # this name is consistent.
   FATAL = "FATAL",
   ERROR = "ERROR",
   WARN  = "WARN",
-  # SUCCESS = "SUCCESS", eg. supported by logger
+  # SUCCESS = "SUCCESS", eg. supported by logger but not by other logging packages. Therefore ignored for now...
   INFO  = "INFO",
   DEBUG = "DEBUG",
   TRACE = "TRACE"

diff --git a/R/log2console.R b/R/log2console.R
@@ -39,12 +39,12 @@ log2console <- function(severity.level, msg) {
 
 
   stopifnot(!is.null(severity.level))
-  stopifnot(severity.level %in% c("ERROR", "WARN", "INFO"))
+  stopifnot(severity.level %in% Severity.Levels)
   stopifnot(is.character(msg))
 
   # Design decision:
   # This simple logging function uses the local time
-  # (not UTC which would allow to combine different log output with
+  # (not UTC which would allow to combine different log outputs with
   #  different time zones more easily).
   log.time <- format(Sys.time(), "%Y-%m-%d %H:%M:%S")
 

diff --git a/R/platform_newline.R b/R/platform_newline.R
@@ -23,6 +23,15 @@
 #'
 #' The newline character(s) are determined once at package loading time.
 #'
+#' You normally don't need to care about this since "\\n" in strings works
+#' on every platform (operating system) because it is converted automatically
+#' when printed, saved in or loaded from a file.
+#'
+#' A typical use case is a unit test that compares an actual file
+#' (eg. with "\\r\\n" on Windows) to an expected file that was eg. created on Linux
+#' with "\\n" platform newline and you do byte compare of the files
+#' (which will fail unless to "undo" the platform newline differences).
+#'
 #' @return the new line character(s) for the current operating system
 #'
 #' @export