From 46b9d02a34d3cbef217233f4926f0a8f09084cf7 Mon Sep 17 00:00:00 2001
From: "Andrew G. Brown" <andrew.g.brown@usda.gov>
Date: Sat, 1 Aug 2020 15:19:40 -0700
Subject: [PATCH] horizons<-: silent re-application of original order and NULL
 protection

---
 R/SoilProfileCollection-setters.R      | 13 ++++++++-----
 R/guessColumnNames.R                   |  2 +-
 tests/testthat/test-DT-tbl.R           |  5 ++---
 tests/testthat/test-SPC-objects.R      |  8 ++++----
 tests/testthat/test-guessColumnNames.R |  4 ++--
 5 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/R/SoilProfileCollection-setters.R b/R/SoilProfileCollection-setters.R
index 87c5c69ef..9b2377078 100644
--- a/R/SoilProfileCollection-setters.R
+++ b/R/SoilProfileCollection-setters.R
@@ -115,13 +115,13 @@ setReplaceMethod("depths", "data.frame",
 
   iddata <- data[[nm[1]]]
   tdep <- data[[depthcols[1]]]
-  
+
   usortid <- unique(sort(iddata))
-  
+
   idtdepord <- order(as.character(iddata), tdep)
   ditd <- data[idtdepord,]
   hsorttdep <- all(ditd[[depthcols[1]]] == tdep)
-  
+
   # re-sort horizon data
   if (suppressWarnings(any(iddata != usortid) | !hsorttdep)) {
     ## note: forced character sort on ID -- need to impose some order to check depths
@@ -460,6 +460,9 @@ if (!isGeneric('horizons<-'))
 setReplaceMethod("horizons", signature(object = "SoilProfileCollection"),
   function(object, value) {
 
+  if (is.null(value))
+    stop("new horizon data must not be NULL; to remove a site or horizon attribute use `spc$attribute <- NULL`", call.=FALSE)
+
   # testing the class of the horizon data to add to the object
   if (!inherits(value, "data.frame"))
 	  stop("new horizon data input value must inherit from data.frame", call.=FALSE)
@@ -504,9 +507,9 @@ setReplaceMethod("horizons", signature(object = "SoilProfileCollection"),
   new.horizon.order <- match(names(original.horizon.order),
                              horizon.new[[hzidname(object)]])
   chnew <- .coalesce.idx(horizon.new[[idname(object)]])
-  if(length(chnew) != length(original.site.order) |
+  if (length(chnew) != length(original.site.order) |
      suppressWarnings(any(original.site.order != chnew))) {
-    message("join condition resulted in sorting of horizons, re-applying original order")
+    # message("join condition resulted in sorting of horizons, re-applying original order")
     horizon.new <- horizon.new[new.horizon.order,]
   }
 
diff --git a/R/guessColumnNames.R b/R/guessColumnNames.R
index cf64d127d..b9be1e068 100644
--- a/R/guessColumnNames.R
+++ b/R/guessColumnNames.R
@@ -97,7 +97,7 @@ guessHzTexClName <- function(x) {
   possible.name1 <- nm[grep('texcl', nm, ignore.case = TRUE)]
 
   # use the first valid guess matching texcl
-  if (length(possible.name1) == 1) {
+  if (length(possible.name1) > 0) {
     possible.name1 <- possible.name1[1]
     return(possible.name1)
   }
diff --git a/tests/testthat/test-DT-tbl.R b/tests/testthat/test-DT-tbl.R
index 6718373c6..13dc5b2bd 100644
--- a/tests/testthat/test-DT-tbl.R
+++ b/tests/testthat/test-DT-tbl.R
@@ -178,10 +178,9 @@ res <- lapply(dfclasses, function(use_class) {
     switch(
       use_class,
       # data.frame and tbl_df
-      expect_message({
+      expect_silent({
         horizons(test) <- value
-      } ,
-      "join condition resulted in sorting of horizons, re-applying original order"),
+      }),
 
       # note: data.table sorts correctly without invoking re-order
       "data.table" = {
diff --git a/tests/testthat/test-SPC-objects.R b/tests/testthat/test-SPC-objects.R
index 109026f6a..52292fa90 100644
--- a/tests/testthat/test-SPC-objects.R
+++ b/tests/testthat/test-SPC-objects.R
@@ -299,15 +299,15 @@ test_that("SPC depth columns get/set ", {
 })
 
 test_that("SPC min/max overrides work as expected", {
-  
+
   set.seed(20202)
   df <- lapply(1:10, random_profile, SPC=TRUE)
   df <- union(df)
-  
+
   ## visually inspect output
   # profileApply(df, min)
   # profileApply(df, max)
-  
+
   # both min and max should return 10cm
   expect_equal(min(df), 44)
   expect_equal(max(df), 134)
@@ -536,7 +536,7 @@ test_that("horizons<- left-join", {
   hnew$prop[1] <- 50
 
   # utilize horizons<- left join
-  expect_message(horizons(x) <- hnew, "join condition resulted in sorting of horizons, re-applying original order")
+  expect_silent(horizons(x) <- hnew)
 
   # verify old columns have same names
   # (i.e. no issues with duplication of column names in merge)
diff --git a/tests/testthat/test-guessColumnNames.R b/tests/testthat/test-guessColumnNames.R
index a8fbf7675..c9cbf5dd7 100644
--- a/tests/testthat/test-guessColumnNames.R
+++ b/tests/testthat/test-guessColumnNames.R
@@ -32,8 +32,8 @@ test_that("basic functionality", {
   expect_equal(guessHzTexClName(sp3), "texcl")
 
   # texture
-  horizons(sp3)$texture <-  horizons(sp3)$texcl
-  horizons(sp3)$texcl <- NULL
+  sp3$texture <-  sp3$texcl
+  sp3$texcl <- NULL
   expect_equal(guessHzTexClName(sp3), "texture")
 
   # descriptive name