From 47d164f24a602269bcee50e7fc3d0eb8ec4dfe44 Mon Sep 17 00:00:00 2001
From: Roland Illig <roland.illig@gmx.de>
Date: Sun, 14 Jul 2024 23:25:11 +0200
Subject: [PATCH] Test trailing space in explanation and errors in log messages

---
 v23/check_test.go   | 12 ++++++++++++
 v23/logging.go      |  7 +++----
 v23/logging_test.go | 18 ++++++++++++++++++
 v23/shell.go        |  2 +-
 4 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/v23/check_test.go b/v23/check_test.go
index b15a901b..41ccc0ca 100644
--- a/v23/check_test.go
+++ b/v23/check_test.go
@@ -1030,6 +1030,18 @@ func (t *Tester) ExpectAssert(action func()) {
 	t.Check(action, check.Panics, "Pkglint internal error")
 }
 
+// ExpectAssert runs the given action and expects that this action calls
+// assertf, failing with the given message.
+//
+// Usage:
+//
+//	t.ExpectAssertf(
+//	    func() { /* do something that panics */ },
+//	    "expected message")
+func (t *Tester) ExpectAssertf(action func(), msg string) {
+	t.Check(action, check.Panics, "Pkglint internal error: "+msg)
+}
+
 // ExpectDiagnosticsAutofix first runs the given action with -Wall, and
 // then another time with -Wall --autofix.
 //
diff --git a/v23/logging.go b/v23/logging.go
index 19260938..276839a4 100644
--- a/v23/logging.go
+++ b/v23/logging.go
@@ -114,15 +114,14 @@ func (l *Logger) Diag(line *Line, level *LogLevel, format string, args ...interf
 	if G.Testing {
 		for _, arg := range args {
 			switch arg.(type) {
-			case int, string:
-			case RelPath:
+			case int, string, RelPath:
 			case CurrPath:
 				// All paths in diagnostics must be relative to the line.
 				// To achieve that, call line.Rel(currPath).
 				_ = arg.(RelPath)
 			case error:
-				// TODO: errors do not belong in diagnostics,
-				//  they belong in normal error messages.
+				// To log an error, use TechFatalF instead.
+				_ = arg.(string)
 			default:
 				_ = arg.(string)
 			}
diff --git a/v23/logging_test.go b/v23/logging_test.go
index 1529e7a3..742b6625 100644
--- a/v23/logging_test.go
+++ b/v23/logging_test.go
@@ -164,6 +164,24 @@ func (s *Suite) Test_Logger_Explain__line_wrapped_temporary_directory(c *check.C
 		"")
 }
 
+func (s *Suite) Test_Logger_Explain__trailing_space(c *check.C) {
+	t := s.Init(c)
+
+	t.SetUpCommandLine("--explain")
+	filename := t.File("filename.mk")
+	mkline := t.NewMkLine(filename, 123, "")
+
+	mkline.Notef("Just a note to get the below explanation.")
+	t.ExpectAssertf(
+		func() {
+			G.Logger.Explain(
+				"Trailing space. ")
+		},
+		"Explanation \"Trailing space. \" must not have trailing space")
+	t.CheckOutputLines(
+		"NOTE: ~/filename.mk:123: Just a note to get the below explanation.")
+}
+
 // Diag filters duplicate messages, unlike Logf.
 func (s *Suite) Test_Logger_Diag__duplicates(c *check.C) {
 	t := s.Init(c)
diff --git a/v23/shell.go b/v23/shell.go
index 33710dae..eb58c793 100644
--- a/v23/shell.go
+++ b/v23/shell.go
@@ -677,7 +677,7 @@ func (ck *ShellLineChecker) CheckShellCommand(shellcmd string, pSetE *bool, time
 		return
 	}
 	if err != nil {
-		line.Warnf("Pkglint ShellLine.CheckShellCommand: %s", err)
+		line.Warnf("Pkglint ShellLine.CheckShellCommand: %s", err.Error())
 		return
 	}