From d6e09505f5c484aed7cc66d69d932ad764513a5d Mon Sep 17 00:00:00 2001
From: per1234 <accounts@perglass.com>
Date: Thu, 10 Dec 2020 17:32:05 -0800
Subject: [PATCH 1/9] Disable logging by default
The log output is not of interest to the ordinary users, so it should only be enabled when one of the logging
configuration environment variables is set.
---
configuration/configuration.go | 6 ++++--
configuration/configuration_test.go | 1 -
configuration/defaults.go | 5 +++--
3 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/configuration/configuration.go b/configuration/configuration.go
index 2e373a4a..a9554d23 100644
--- a/configuration/configuration.go
+++ b/configuration/configuration.go
@@ -56,12 +56,15 @@ func Initialize(flags *pflag.FlagSet, projectPaths []string) error {
}
}
+ logrus.SetOutput(defaultLogOutput)
+
if logFormatString, ok := os.LookupEnv("ARDUINO_CHECK_LOG_FORMAT"); ok {
logFormat, err := logFormatFromString(logFormatString)
if err != nil {
return fmt.Errorf("--log-format flag value %s not valid", logFormatString)
}
logrus.SetFormatter(logFormat)
+ logrus.SetOutput(os.Stderr) // Enable log output.
}
if logLevelString, ok := os.LookupEnv("ARDUINO_CHECK_LOG_LEVEL"); ok {
@@ -70,8 +73,7 @@ func Initialize(flags *pflag.FlagSet, projectPaths []string) error {
return fmt.Errorf("--log-level flag value %s not valid", logLevelString)
}
logrus.SetLevel(logLevel)
- } else {
- logrus.SetLevel(defaultLogLevel)
+ logrus.SetOutput(os.Stderr) // Enable log output.
}
superprojectTypeFilterString, _ := flags.GetString("project-type")
diff --git a/configuration/configuration_test.go b/configuration/configuration_test.go
index 9ea49f30..b44e0b6f 100644
--- a/configuration/configuration_test.go
+++ b/configuration/configuration_test.go
@@ -120,7 +120,6 @@ func TestInitializeLogFormat(t *testing.T) {
func TestInitializeLogLevel(t *testing.T) {
require.Nil(t, Initialize(test.ConfigurationFlags(), projectPaths))
- assert.Equal(t, defaultLogLevel, logrus.GetLevel(), "Default level")
os.Setenv("ARDUINO_CHECK_LOG_LEVEL", "foo")
assert.Error(t, Initialize(test.ConfigurationFlags(), projectPaths), "Invalid level")
diff --git a/configuration/defaults.go b/configuration/defaults.go
index a4cc1e3b..7f2f3d5d 100644
--- a/configuration/defaults.go
+++ b/configuration/defaults.go
@@ -18,9 +18,10 @@ package configuration
// The default configuration settings.
import (
+ "io/ioutil"
+
"github.com/arduino/arduino-check/configuration/checkmode"
"github.com/arduino/arduino-check/project/projecttype"
- "github.com/sirupsen/logrus"
)
// Default check modes for each superproject type.
@@ -60,4 +61,4 @@ var defaultCheckModes = map[projecttype.Type]map[checkmode.Type]bool{
},
}
-var defaultLogLevel = logrus.FatalLevel
+var defaultLogOutput = ioutil.Discard // Default to no log output.
From e4852b105e56c84dfc913d7e11e4dfc1f7a92cc6 Mon Sep 17 00:00:00 2001
From: per1234 <accounts@perglass.com>
Date: Thu, 10 Dec 2020 17:38:11 -0800
Subject: [PATCH 2/9] Make feedback.Error*() print to stderr
---
result/feedback/feedback.go | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/result/feedback/feedback.go b/result/feedback/feedback.go
index bf603aa1..17e1b87a 100644
--- a/result/feedback/feedback.go
+++ b/result/feedback/feedback.go
@@ -18,6 +18,7 @@ package feedback
import (
"fmt"
+ "os"
"github.com/arduino/arduino-check/configuration"
"github.com/arduino/arduino-check/result/outputformat"
@@ -55,6 +56,6 @@ func Errorf(format string, v ...interface{}) {
// Error behaves like fmt.Print but also logs the error.
func Error(errorMessage string) {
- fmt.Printf(errorMessage)
+ fmt.Fprint(os.Stderr, errorMessage)
logrus.Error(fmt.Sprint(errorMessage))
}
From 2145f352ca4756ea60943fcf7a3fbd9dc0071d25 Mon Sep 17 00:00:00 2001
From: per1234 <accounts@perglass.com>
Date: Thu, 10 Dec 2020 17:39:45 -0800
Subject: [PATCH 3/9] Make feedback.Error*() add a newline to the output
The error must always be printed with a newline, otherwise the log will be appended to the output.
This follows the convention set by the Arduino CLI feedback package.
---
result/feedback/feedback.go | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/result/feedback/feedback.go b/result/feedback/feedback.go
index 17e1b87a..df033d52 100644
--- a/result/feedback/feedback.go
+++ b/result/feedback/feedback.go
@@ -49,13 +49,13 @@ func Print(message string) {
}
}
-// Errorf behaves like fmt.Printf but also logs the error.
+// Errorf behaves like fmt.Printf but adds a newline and also logs the error.
func Errorf(format string, v ...interface{}) {
Error(fmt.Sprintf(format, v...))
}
-// Error behaves like fmt.Print but also logs the error.
+// Error behaves like fmt.Print but adds a newline and also logs the error.
func Error(errorMessage string) {
- fmt.Fprint(os.Stderr, errorMessage)
+ fmt.Fprintln(os.Stderr, errorMessage)
logrus.Error(fmt.Sprint(errorMessage))
}
From eb96078420f754a52bcf0812edf4ab29cfe7f351 Mon Sep 17 00:00:00 2001
From: per1234 <accounts@perglass.com>
Date: Thu, 10 Dec 2020 17:45:14 -0800
Subject: [PATCH 4/9] Change parameter of feedback package functions to
variadic interface
Even though not currently needed, this brings consistency with the standard fmt API.
---
result/feedback/feedback.go | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/result/feedback/feedback.go b/result/feedback/feedback.go
index df033d52..770c5bcc 100644
--- a/result/feedback/feedback.go
+++ b/result/feedback/feedback.go
@@ -31,9 +31,9 @@ func VerbosePrintf(format string, v ...interface{}) {
}
// VerbosePrint behaves like Print but only prints when verbosity is enabled.
-func VerbosePrint(message string) {
+func VerbosePrint(v ...interface{}) {
if configuration.Verbose() && (configuration.OutputFormat() == outputformat.Text) {
- Printf(message)
+ Print(v...)
}
}
@@ -43,9 +43,9 @@ func Printf(format string, v ...interface{}) {
}
// Print behaves like fmt.Print but only prints when output format is set to `text`.
-func Print(message string) {
+func Print(v ...interface{}) {
if configuration.OutputFormat() == outputformat.Text {
- fmt.Printf(message)
+ fmt.Print(v...)
}
}
@@ -55,7 +55,7 @@ func Errorf(format string, v ...interface{}) {
}
// Error behaves like fmt.Print but adds a newline and also logs the error.
-func Error(errorMessage string) {
- fmt.Fprintln(os.Stderr, errorMessage)
- logrus.Error(fmt.Sprint(errorMessage))
+func Error(v ...interface{}) {
+ fmt.Fprintln(os.Stderr, v...)
+ logrus.Error(fmt.Sprint(v...))
}
From 3e6d2f69213ef4855858f23dcb1a8a69e0792914 Mon Sep 17 00:00:00 2001
From: per1234 <accounts@perglass.com>
Date: Thu, 10 Dec 2020 17:47:55 -0800
Subject: [PATCH 5/9] Add *ln functions to feedback package
---
result/feedback/feedback.go | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/result/feedback/feedback.go b/result/feedback/feedback.go
index 770c5bcc..da79d815 100644
--- a/result/feedback/feedback.go
+++ b/result/feedback/feedback.go
@@ -25,6 +25,12 @@ import (
"github.com/sirupsen/logrus"
)
+// VerbosePrintln behaves like Println but only prints when verbosity is enabled.
+func VerbosePrintln(v ...interface{}) {
+ VerbosePrint(v...)
+ VerbosePrint("\n")
+}
+
// VerbosePrintf behaves like Printf but only prints when verbosity is enabled.
func VerbosePrintf(format string, v ...interface{}) {
VerbosePrint(fmt.Sprintf(format, v...))
@@ -37,6 +43,12 @@ func VerbosePrint(v ...interface{}) {
}
}
+// Println behaves like fmt.Println but only prints when output format is set to `text`.
+func Println(v ...interface{}) {
+ Print(v...)
+ Print("\n")
+}
+
// Printf behaves like fmt.Printf but only prints when output format is set to `text`.
func Printf(format string, v ...interface{}) {
Print(fmt.Sprintf(format, v...))
From f636b764fcf34dd6767b7f9adcd1886b2c2a87aa Mon Sep 17 00:00:00 2001
From: per1234 <accounts@perglass.com>
Date: Thu, 10 Dec 2020 18:03:27 -0800
Subject: [PATCH 6/9] Add trailing newlines at the point of output
The previous approach of adding trailing newlines when the report text was generated made it difficult to keep track of
how the output was formatted.
---
check/check.go | 6 +++---
command/command.go | 2 +-
result/result.go | 8 ++++----
result/result_test.go | 8 ++++----
4 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/check/check.go b/check/check.go
index a659622d..ef12c3f4 100644
--- a/check/check.go
+++ b/check/check.go
@@ -33,7 +33,7 @@ import (
// RunChecks runs all checks for the given project and outputs the results.
func RunChecks(project project.Type) {
- feedback.Printf("Checking %s in %s\n", project.ProjectType, project.Path)
+ feedback.Printf("\nChecking %s in %s\n", project.ProjectType, project.Path)
checkdata.Initialize(project, configuration.SchemasPath())
@@ -55,7 +55,7 @@ func RunChecks(project project.Type) {
checkResult, checkOutput := checkConfiguration.CheckFunction()
reportText := result.Results.Record(project, checkConfiguration, checkResult, checkOutput)
if (checkResult == checkresult.Fail) || configuration.Verbose() {
- feedback.Print(reportText)
+ feedback.Println(reportText)
}
}
@@ -63,7 +63,7 @@ func RunChecks(project project.Type) {
result.Results.AddProjectSummary(project)
// Print the project check results summary.
- feedback.Print(result.Results.ProjectSummaryText(project))
+ feedback.Printf("\n%s\n", result.Results.ProjectSummaryText(project))
}
// shouldRun returns whether a given check should be run for the given project under the current tool configuration.
diff --git a/command/command.go b/command/command.go
index cb6826b9..c6f14c19 100644
--- a/command/command.go
+++ b/command/command.go
@@ -75,7 +75,7 @@ func ArduinoCheck(rootCommand *cobra.Command, cliArguments []string) {
if configuration.OutputFormat() == outputformat.Text {
if len(projects) > 1 {
// There are multiple projects, print the summary of check results for all projects.
- fmt.Print(result.Results.SummaryText())
+ fmt.Printf("\n%s\n", result.Results.SummaryText())
}
} else {
// Print the complete JSON formatted report.
diff --git a/result/result.go b/result/result.go
index 61ff2709..8d901548 100644
--- a/result/result.go
+++ b/result/result.go
@@ -99,7 +99,7 @@ func (results *Type) Record(checkedProject project.Type, checkConfiguration chec
os.Exit(1)
}
- summaryText := fmt.Sprintf("Check %s result: %s\n", checkConfiguration.ID, checkResult)
+ summaryText := fmt.Sprintf("Check %s result: %s", checkConfiguration.ID, checkResult)
checkMessage := ""
if checkResult == checkresult.Fail {
@@ -112,7 +112,7 @@ func (results *Type) Record(checkedProject project.Type, checkConfiguration chec
// Add explanation of check result if present.
if checkMessage != "" {
- summaryText += fmt.Sprintf("%s: %s\n", checkLevel, checkMessage)
+ summaryText += fmt.Sprintf("\n%s: %s", checkLevel, checkMessage)
}
reportExists, projectReportIndex := results.getProjectReportIndex(checkedProject.Path)
@@ -187,7 +187,7 @@ func (results Type) ProjectSummaryText(checkedProject project.Type) string {
}
projectSummaryReport := results.Projects[projectReportIndex].Summary
- return fmt.Sprintf("\nFinished checking project. Results:\nWarning count: %v\nError count: %v\nChecks passed: %v\n\n", projectSummaryReport.WarningCount, projectSummaryReport.ErrorCount, projectSummaryReport.Pass)
+ return fmt.Sprintf("Finished checking project. Results:\nWarning count: %v\nError count: %v\nChecks passed: %v", projectSummaryReport.WarningCount, projectSummaryReport.ErrorCount, projectSummaryReport.Pass)
}
// AddSummary summarizes the check results for all projects and adds it to the report.
@@ -212,7 +212,7 @@ func (results *Type) AddSummary() {
// SummaryText returns a text summary of the cumulative check results.
func (results Type) SummaryText() string {
- return fmt.Sprintf("Finished checking projects. Results:\nWarning count: %v\nError count: %v\nChecks passed: %v\n", results.Summary.WarningCount, results.Summary.ErrorCount, results.Summary.Pass)
+ return fmt.Sprintf("Finished checking projects. Results:\nWarning count: %v\nError count: %v\nChecks passed: %v", results.Summary.WarningCount, results.Summary.ErrorCount, results.Summary.Pass)
}
// JSONReport returns a JSON formatted report of checks on all projects.
diff --git a/result/result_test.go b/result/result_test.go
index bab43d5a..3892cac4 100644
--- a/result/result_test.go
+++ b/result/result_test.go
@@ -75,9 +75,9 @@ func TestRecord(t *testing.T) {
checkConfiguration := checkconfigurations.Configurations()[0]
checkOutput := "foo"
summaryText := results.Record(checkedProject, checkConfiguration, checkresult.Fail, checkOutput)
- assert.Equal(t, fmt.Sprintf("Check %s result: %s\n%s: %s\n", checkConfiguration.ID, checkresult.Fail, checklevel.Error, message(checkConfiguration.MessageTemplate, checkOutput)), summaryText)
+ assert.Equal(t, fmt.Sprintf("Check %s result: %s\n%s: %s", checkConfiguration.ID, checkresult.Fail, checklevel.Error, message(checkConfiguration.MessageTemplate, checkOutput)), summaryText)
summaryText = results.Record(checkedProject, checkConfiguration, checkresult.NotRun, checkOutput)
- assert.Equal(t, fmt.Sprintf("Check %s result: %s\n%s: %s\n", checkConfiguration.ID, checkresult.NotRun, checklevel.Notice, checkOutput), summaryText, "Non-fail result should not use message")
+ assert.Equal(t, fmt.Sprintf("Check %s result: %s\n%s: %s", checkConfiguration.ID, checkresult.NotRun, checklevel.Notice, checkOutput), summaryText, "Non-fail result should not use message")
summaryText = results.Record(checkedProject, checkConfiguration, checkresult.Pass, "")
assert.Equal(t, "", "", summaryText, "Non-failure result with no check function output should result in an empty summary")
@@ -198,7 +198,7 @@ func TestAddProjectSummary(t *testing.T) {
assert.Equal(t, testTable.expectedPass, results.Projects[0].Summary.Pass)
assert.Equal(t, testTable.expectedWarningCount, results.Projects[0].Summary.WarningCount)
assert.Equal(t, testTable.expectedErrorCount, results.Projects[0].Summary.ErrorCount)
- assert.Equal(t, fmt.Sprintf("\nFinished checking project. Results:\nWarning count: %v\nError count: %v\nChecks passed: %v\n\n", testTable.expectedWarningCount, testTable.expectedErrorCount, testTable.expectedPass), results.ProjectSummaryText(checkedProject))
+ assert.Equal(t, fmt.Sprintf("Finished checking project. Results:\nWarning count: %v\nError count: %v\nChecks passed: %v", testTable.expectedWarningCount, testTable.expectedErrorCount, testTable.expectedPass), results.ProjectSummaryText(checkedProject))
}
}
@@ -281,7 +281,7 @@ func TestAddSummary(t *testing.T) {
assert.Equal(t, testTable.expectedPass, results.Passed())
assert.Equal(t, testTable.expectedWarningCount, results.Summary.WarningCount)
assert.Equal(t, testTable.expectedErrorCount, results.Summary.ErrorCount)
- assert.Equal(t, fmt.Sprintf("Finished checking projects. Results:\nWarning count: %v\nError count: %v\nChecks passed: %v\n", testTable.expectedWarningCount, testTable.expectedErrorCount, testTable.expectedPass), results.SummaryText())
+ assert.Equal(t, fmt.Sprintf("Finished checking projects. Results:\nWarning count: %v\nError count: %v\nChecks passed: %v", testTable.expectedWarningCount, testTable.expectedErrorCount, testTable.expectedPass), results.SummaryText())
}
}
From 5421e9f5995f571ba075f75f55d733082a3eb549 Mon Sep 17 00:00:00 2001
From: per1234 <accounts@perglass.com>
Date: Thu, 10 Dec 2020 18:06:21 -0800
Subject: [PATCH 7/9] Move project summary report output out of
check.RunChecks()
It's better for this function to be exclusively for the running of checks. It is more sensible for the project related
output to be handled at a higher level.
---
check/check.go | 6 ------
command/command.go | 6 ++++++
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/check/check.go b/check/check.go
index ef12c3f4..796e3217 100644
--- a/check/check.go
+++ b/check/check.go
@@ -58,12 +58,6 @@ func RunChecks(project project.Type) {
feedback.Println(reportText)
}
}
-
- // Checks are finished for this project, so summarize its check results in the report.
- result.Results.AddProjectSummary(project)
-
- // Print the project check results summary.
- feedback.Printf("\n%s\n", result.Results.ProjectSummaryText(project))
}
// shouldRun returns whether a given check should be run for the given project under the current tool configuration.
diff --git a/command/command.go b/command/command.go
index c6f14c19..13e453b1 100644
--- a/command/command.go
+++ b/command/command.go
@@ -67,6 +67,12 @@ func ArduinoCheck(rootCommand *cobra.Command, cliArguments []string) {
for _, project := range projects {
check.RunChecks(project)
+
+ // Checks are finished for this project, so summarize its check results in the report.
+ result.Results.AddProjectSummary(project)
+
+ // Print the project check results summary.
+ feedback.Printf("\n%s\n", result.Results.ProjectSummaryText(project))
}
// All projects have been checked, so summarize their check results in the report.
From d6701acf7438473f6be6591e951222ced1d3b8bb Mon Sep 17 00:00:00 2001
From: per1234 <accounts@perglass.com>
Date: Thu, 10 Dec 2020 18:08:36 -0800
Subject: [PATCH 8/9] Always panic on internal failures
These uses of `feedback.Error()` in the event of internal failures was a departure from the convention established in
the rest of the code of using `feedback.Error()` only for failures resulting from external issues, and panic() for those
resulting from internal issues (in which case the traceback is useful).
---
check/check.go | 4 +---
result/result.go | 5 +----
2 files changed, 2 insertions(+), 7 deletions(-)
diff --git a/check/check.go b/check/check.go
index 796e3217..c98a391e 100644
--- a/check/check.go
+++ b/check/check.go
@@ -18,7 +18,6 @@ package check
import (
"fmt"
- "os"
"github.com/arduino/arduino-check/check/checkconfigurations"
"github.com/arduino/arduino-check/check/checkdata"
@@ -40,8 +39,7 @@ func RunChecks(project project.Type) {
for _, checkConfiguration := range checkconfigurations.Configurations() {
runCheck, err := shouldRun(checkConfiguration, project)
if err != nil {
- feedback.Errorf("Error while determining whether to run check: %v", err)
- os.Exit(1)
+ panic(err)
}
if !runCheck {
diff --git a/result/result.go b/result/result.go
index 8d901548..c8b72b4f 100644
--- a/result/result.go
+++ b/result/result.go
@@ -21,7 +21,6 @@ import (
"encoding/json"
"fmt"
"html/template"
- "os"
"github.com/arduino/arduino-check/check/checkconfigurations"
"github.com/arduino/arduino-check/check/checklevel"
@@ -29,7 +28,6 @@ import (
"github.com/arduino/arduino-check/configuration"
"github.com/arduino/arduino-check/configuration/checkmode"
"github.com/arduino/arduino-check/project"
- "github.com/arduino/arduino-check/result/feedback"
"github.com/arduino/go-paths-helper"
)
@@ -95,8 +93,7 @@ func (results *Type) Initialize() {
func (results *Type) Record(checkedProject project.Type, checkConfiguration checkconfigurations.Type, checkResult checkresult.Type, checkOutput string) string {
checkLevel, err := checklevel.CheckLevel(checkConfiguration, checkResult)
if err != nil {
- feedback.Errorf("Error while determining check level: %v", err)
- os.Exit(1)
+ panic(fmt.Errorf("Error while determining check level: %v", err))
}
summaryText := fmt.Sprintf("Check %s result: %s", checkConfiguration.ID, checkResult)
From 23d32fd1a16193bf00e2d08eb3ad4f6977e3f132 Mon Sep 17 00:00:00 2001
From: per1234 <accounts@perglass.com>
Date: Thu, 10 Dec 2020 18:34:46 -0800
Subject: [PATCH 9/9] Move log output to after error check
This log output is only meaningful if there wasn't an error.
---
check/checkdata/schema/parsevalidationresult.go | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/check/checkdata/schema/parsevalidationresult.go b/check/checkdata/schema/parsevalidationresult.go
index f358ef54..b1db7af5 100644
--- a/check/checkdata/schema/parsevalidationresult.go
+++ b/check/checkdata/schema/parsevalidationresult.go
@@ -189,10 +189,10 @@ func validationErrorSchemaPointerValueMatch(
schemasPath *paths.Path,
) bool {
marshalledSchemaPointerValue, err := json.Marshal(schemaPointerValue(schemaURL, schemaPointer, schemasPath))
- logrus.Tracef("Checking schema pointer value: %s match with regexp: %s", marshalledSchemaPointerValue, schemaPointerValueRegexp)
if err != nil {
panic(err)
}
+ logrus.Tracef("Checking schema pointer value: %s match with regexp: %s", marshalledSchemaPointerValue, schemaPointerValueRegexp)
return schemaPointerValueRegexp.Match(marshalledSchemaPointerValue)
}