Fix #208: validate units before installation

NEWS.md

@@ -2,6 +2,8 @@
 
 * fix support for weights with units in `weighted.mean`; #205
 
+* invalid names for new units now trigger a proper error message; #209 addressing #208
+
 # version 0.6-3
 
 * improve platform dependent encodings handling; #183

R/deprecated.R

@@ -9,7 +9,7 @@
 #' @rdname deprecated
 make_unit <- function(chr) {
   .Deprecated("as_units", msg = "make_unit() is deprecated. Please use as_units()")
-  if (! is_valid_unit_symbol(chr))
+  if (!ud_is_parseable(chr))
     warning(paste(sQuote(chr), "is not a valid unit symbol recognized by udunits"), call.=FALSE)
   as_units.character(chr, force_single_symbol = TRUE, check_is_valid = FALSE)
 }

R/make_units.R

@@ -296,10 +296,6 @@ pc_and <- function(..., sep = "") {
     "Prefixes will automatically work with any user-defined unit.")
 }
 
-is_valid_unit_symbol <- function(chr) {
-  ud_is_parseable(chr)
-}
-
 units_eval_env <- new.env(parent = baseenv())
 units_eval_env$ln <- function(x) base::log(x)
 units_eval_env$lg <- function(x) base::log(x, base = 10)
@@ -350,7 +346,7 @@ as_units.call <- function(x, check_is_valid = TRUE, ...) {
 See ?as_units for usage examples.")
 
   if (check_is_valid) {
-    valid <- vapply(vars, is_valid_unit_symbol, logical(1L))
+    valid <- vapply(vars, ud_is_parseable, logical(1L))
     if (!all(valid))
       stop(.msg_units_not_recognized(vars[!valid], x), call. = FALSE)
   }
@@ -388,7 +384,7 @@ symbolic_unit <- function(chr, check_is_valid = TRUE) {
 
   stopifnot(is.character(chr), length(chr) == 1)
 
-  if (check_is_valid && !is_valid_unit_symbol(chr)) {
+  if (check_is_valid && !ud_is_parseable(chr)) {
     msg <- paste(sQuote(chr), "is not a unit recognized by udunits or a user-defined unit")
     stop(msg, call. = FALSE)
   }

R/user_conversion.R

@@ -17,6 +17,7 @@
 #' install_symbolic_unit("person")
 #' set_units(1, rad) + set_units(1, person) # that is how dimensionless units work!
 install_symbolic_unit <- function(name, warn = TRUE, dimensionless = TRUE) {
+  check_unit_format(name)
   if(ud_is_parseable(name)) {
     if (warn) 
       warning(sQuote(name), 
@@ -68,9 +69,9 @@ install_conversion_constant <- function(from, to, const) {
   if (! xor(ud_is_parseable(from), ud_is_parseable(to)))
     stop("exactly one of (from, to) must be a known unit")
   if (ud_is_parseable(to))
-  	R_ut_scale(as.character(from), as.character(to), as.double(const))
+    R_ut_scale(check_unit_format(from), to, as.double(const))
   else
-    R_ut_scale(as.character(to), as.character(from), 1.0 / as.double(const))
+    R_ut_scale(check_unit_format(to), from, 1.0 / as.double(const))
 }
 
 #' @export
@@ -86,7 +87,24 @@ install_conversion_offset <- function(from, to, const) {
   if (! xor(ud_is_parseable(from), ud_is_parseable(to)))
     stop("exactly one of (from, to) must be a known unit")
   if (ud_is_parseable(to))
-    R_ut_offset(as.character(from), as.character(to), -as.double(const))
+    R_ut_offset(check_unit_format(from), to, -as.double(const))
   else
-    R_ut_offset(as.character(to), as.character(from), as.double(const))
+    R_ut_offset(check_unit_format(to), from, as.double(const))
+}
+
+check_unit_format <- function(x) {
+  cond <- c(
+    # leading and trailing numbers
+    grepl("^[[:space:]]*[0-9]+", x), grepl("[0-9]+[[:space:]]*$", x),
+    # arithmetic operators
+    grepl("\\+|\\-|\\*|\\/|\\^", x),
+    # intermediate spaces
+    grepl("[[:alnum:][:punct:]]+[[:space:]]+[[:alnum:][:punct:]]+", x)
+  )
+  if (any(cond))
+    stop("the following elements are not allowed in new unit names/symbols:\n",
+         "  - leading or trailing numbers\n",
+         "  - arithmetic operators\n",
+         "  - intermediate white spaces")
+  x
 }

tests/testthat/test_user_conversion.R

@@ -61,3 +61,17 @@ test_that("removing units works", {
   expect_silent(remove_symbolic_unit("foo"))
   expect_error(remove_symbolic_unit("foo"))
 })
+
+test_that("new units' format is checked for possible issues", {
+  wrong_formats <- c(
+    " 2asdf", "asdf2 ", 
+    "as+df", "as-df", "as*df", "as/df", "as^df",
+    "as df")
+  for (i in wrong_formats) {
+    expect_error(install_symbolic_unit(i))
+    expect_error(install_conversion_constant(i, "m", 2))
+    expect_error(install_conversion_constant("m", i, 2))
+    expect_error(install_conversion_offset(i, "m", 2))
+    expect_error(install_conversion_offset("m", i, 2))
+  }
+})