[breaking] legacy: refactoring of the old i18n.Logger (part 2) (#1625)

* legacy: refactored ErrorfWithLogger function

Now is no longer needed, this function was basically made for
arduino-builder to allow transfering the error to the IDE, BTW the
correct fix must be made inside arduino-builder (to actually let him
push the error in the logger).

* Renamed legacy ctx fields ExecStdout/ExecStderr to Stdout/Stderr

* Removed empty Lint() function

* Removed dependency on i18n.Logger

* Removed no more used i18n.Logger \o/ \o/

* Simplified i18n.Init function

* legacy: builder default output on os.Stdout/os.Stderr

* legacy: updated integration tests for slightly different output of builder

* Use positional parameter for most translated string

This will make easier to transfer translations from the Arduino IDE 1.8

* Added note to UPGRADING.md

* Updated UPGRADING.md

* Removed extra blank line in library detection recap

* Update docs/UPGRADING.md

Co-authored-by: per1234 <accounts@perglass.com>

Author: cmaglie

arduino/libraries/lint.go

@@ -33,7 +33,6 @@ import (
 	"github.com/arduino/arduino-cli/configuration"
 	"github.com/arduino/arduino-cli/i18n"
 	"github.com/arduino/arduino-cli/legacy/builder"
-	legacyi18n "github.com/arduino/arduino-cli/legacy/builder/i18n"
 	"github.com/arduino/arduino-cli/legacy/builder/types"
 	"github.com/arduino/arduino-cli/metrics"
 	rpc "github.com/arduino/arduino-cli/rpc/cc/arduino/cli/commands/v1"
@@ -167,12 +166,6 @@ func Compile(ctx context.Context, req *rpc.CompileRequest, outStream, errStream
 	builderCtx.USBVidPid = req.GetVidPid()
 	builderCtx.WarningsLevel = req.GetWarnings()
 
-	if debug {
-		builderCtx.DebugLevel = 100
-	} else {
-		builderCtx.DebugLevel = 5
-	}
-
 	builderCtx.CustomBuildProperties = append(req.GetBuildProperties(), "build.warn_data_percentage=75")
 
 	if req.GetBuildCachePath() != "" {
@@ -205,9 +198,8 @@ func Compile(ctx context.Context, req *rpc.CompileRequest, outStream, errStream
 		builderCtx.BuiltInLibrariesDirs = paths.NewPathList(ideLibrariesPath)
 	}
 
-	builderCtx.ExecStdout = outStream
-	builderCtx.ExecStderr = errStream
-	builderCtx.SetLogger(&legacyi18n.LoggerToCustomStreams{Stdout: outStream, Stderr: errStream})
+	builderCtx.Stdout = outStream
+	builderCtx.Stderr = errStream
 	builderCtx.Clean = req.GetClean()
 	builderCtx.OnlyUpdateCompilationDatabase = req.GetCreateCompilationDatabaseOnly()
 

commands/compile/compile.go

@@ -32,6 +32,29 @@ The `parseArch` parameter was removed since it was unused and was always true. T
 always parsed by the function. Furthermore the function now should also correctly interpret `packager:arch` spelled with
 the wrong casing.
 
+### `github.com/arduino/arduino-cli/i18n.Init(...)` now requires an empty string to be passed for autodetection of locale
+
+For automated detection of locale, change the call from:
+
+```go
+i18n.Init()
+```
+
+to
+
+```go
+i18n.Init("")
+```
+
+### `github.com/arduino/arduino-cli/legacy/i18n` module has been removed (in particular the `i18n.Logger`)
+
+The `i18n.Logger` is no longer available. It was mainly used in the legacy builder struct field `Context.Logger`.
+
+The `Context.Logger` field has been replaced with plain `io.Writer` fields `Contex.Stdout` and `Context.Stderr`. All
+existing logger functionality has been dropped, for example the Java-Style formatting with tags like `{0} {1}...` must
+be replaced with one of the equivalent golang printf-based alternatives and logging levels must be replaced with direct
+writes to `Stdout` or `Stderr`.
+
 ## 0.20.0
 
 ### `board details` arguments change

docs/UPGRADING.md

@@ -0,0 +1,44 @@
+// This file is part of arduino-cli.
+//
+// Copyright 2020 ARDUINO SA (http://www.arduino.cc/)
+//
+// This software is released under the GNU General Public License version 3,
+// which covers the main part of arduino-cli.
+// The terms of this license can be found at:
+// https://www.gnu.org/licenses/gpl-3.0.en.html
+//
+// You can be released from the requirements of the above licenses by purchasing
+// a commercial license. Buying such a license is mandatory if you want to
+// modify or otherwise use the software for commercial activities involving the
+// Arduino software without disclosing the source code of your own applications.
+// To purchase a commercial license, send an email to license@arduino.cc.
+
+package i18n
+
+import (
+	"regexp"
+	"strconv"
+	"strings"
+)
+
+var javaFormatPlaceholderRegexp = regexp.MustCompile(`{(\d)}`)
+
+// FromJavaToGoSyntax convert a translation string made for Java to a one suitable for golang (printf-style).
+// The conversion transforms java placeholders like "{0}","{1}","{2}",etc... with the equivalent for golang
+// "%[1]v","%[2]v","%[3]v",etc...
+// The double single-quote "''" is translated into a single single-quote "'".
+func FromJavaToGoSyntax(s string) string {
+	// Replace "{x}" => "%[x+1]v"
+	for _, submatch := range javaFormatPlaceholderRegexp.FindAllStringSubmatch(s, -1) {
+		idx, err := strconv.Atoi(submatch[1])
+		if err != nil {
+			panic(err)
+		}
+		s = strings.Replace(s, submatch[0], "%["+strconv.Itoa(idx+1)+"]v", -1)
+	}
+
+	// Replace "''" => "'"
+	s = strings.Replace(s, "''", "'", -1)
+
+	return s
+}

i18n/convert.go

@@ -0,0 +1,54 @@
+// This file is part of arduino-cli.
+//
+// Copyright 2020 ARDUINO SA (http://www.arduino.cc/)
+//
+// This software is released under the GNU General Public License version 3,
+// which covers the main part of arduino-cli.
+// The terms of this license can be found at:
+// https://www.gnu.org/licenses/gpl-3.0.en.html
+//
+// You can be released from the requirements of the above licenses by purchasing
+// a commercial license. Buying such a license is mandatory if you want to
+// modify or otherwise use the software for commercial activities involving the
+// Arduino software without disclosing the source code of your own applications.
+// To purchase a commercial license, send an email to license@arduino.cc.
+
+package i18n
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func format(format string, a ...interface{}) string {
+	format = FromJavaToGoSyntax(format)
+	message := fmt.Sprintf(format, a...)
+	return message
+}
+
+func TestI18NSyntax(t *testing.T) {
+	require.Equal(t, "Do you want to remove %[1]v?\nIf you do so you won't be able to use %[1]v any more.", FromJavaToGoSyntax("Do you want to remove {0}?\nIf you do so you won't be able to use {0} any more."))
+	require.Equal(t, "A file named \"%[1]v\" already exists in \"%[2]v\"", FromJavaToGoSyntax("A file named \"{0}\" already exists in \"{1}\""))
+	require.Equal(t, "Board %[1]v:%[2]v:%[3]v doesn't define a 'build.board' preference. Auto-set to: %[4]v", FromJavaToGoSyntax("Board {0}:{1}:{2} doesn''t define a ''build.board'' preference. Auto-set to: {3}"))
+
+	require.Equal(t, "22 11\n", fmt.Sprintf("%[2]d %[1]d\n", 11, 22))
+	require.Equal(t, "d c b a", format("{3} {2} {1} {0}", "a", "b", "c", "d"))
+
+	require.Equal(t, "e d b a", format("{4} {3} {1} {0}", "a", "b", "c", "d", "e"))
+
+	require.Equal(t, "a b", format("{0} {1}", "a", "b", "c", "d", "e"))
+
+	require.Equal(t, "%!v(BADINDEX) c b a", format("{3} {2} {1} {0}", "a", "b", "c"))
+	require.Equal(t, "%!v(BADINDEX) %!v(BADINDEX) %!v(BADINDEX) %!v(BADINDEX)", format("{3} {2} {1} {0}"))
+
+	require.Equal(t, "I'm %[1]v%% sure", FromJavaToGoSyntax("I'm {0}%% sure"))
+	require.Equal(t, "I'm 100% sure", format("I'm {0}%% sure", 100))
+
+	require.Equal(t, "Either in [1] or in [2]", format("Either in [{0}] or in [{1}]", 1, 2))
+
+	require.Equal(t, "Using library a at version b in folder: c ", format("Using library {0} at version {1} in folder: {2} {3}", "a", "b", "c", ""))
+
+	require.Equal(t, "Missing 'a' from library in b", format("Missing '{0}' from library in {1}", "a", "b"))
+}

i18n/convert_test.go

@@ -19,14 +19,10 @@ package i18n
 // 1. Locale specified via the function call
 // 2. OS Locale
 // 3. en (default)
-func Init(configLocale ...string) {
+func Init(configLocale string) {
 	locales := supportedLocales()
-	if len(configLocale) > 1 {
-		panic("Multiple arguments not supported")
-	}
-
-	if len(configLocale) > 0 && configLocale[0] != "" {
-		if locale := findMatchingLocale(configLocale[0], locales); locale != "" {
+	if configLocale != "" {
+		if locale := findMatchingLocale(configLocale, locales); locale != "" {
 			setLocale(locale)
 			return
 		}

i18n/i18n.go

@@ -16,7 +16,6 @@
 package builder
 
 import (
-	"os"
 	"strings"
 
 	"github.com/arduino/arduino-cli/legacy/builder/constants"
@@ -27,22 +26,17 @@ type AddBuildBoardPropertyIfMissing struct{}
 
 func (*AddBuildBoardPropertyIfMissing) Run(ctx *types.Context) error {
 	packages := ctx.Hardware
-	logger := ctx.GetLogger()
 
 	for _, aPackage := range packages {
 		for _, platform := range aPackage.Platforms {
 			for _, platformRelease := range platform.Releases {
 				for _, board := range platformRelease.Boards {
 					if board.Properties.Get("build.board") == "" {
 						board.Properties.Set("build.board", strings.ToUpper(platform.Architecture+"_"+board.BoardID))
-						logger.Fprintln(
-							os.Stdout,
-							constants.LOG_LEVEL_WARN,
-							tr("Warning: Board {0}:{1}:{2} doesn''t define a %s preference. Auto-set to: {3}", "''build.board''"),
-							aPackage.Name,
-							platform.Architecture,
-							board.BoardID,
-							board.Properties.Get(constants.BUILD_PROPERTIES_BUILD_BOARD))
+						ctx.Info(tr("Warning: Board %[1]s doesn't define a %[2]s preference. Auto-set to: %[3]s",
+							aPackage.Name+":"+platform.Architecture+":"+board.BoardID,
+							"'build.board'",
+							board.Properties.Get(constants.BUILD_PROPERTIES_BUILD_BOARD)))
 					}
 				}
 			}

legacy/builder/add_build_board_property_if_missing.go

@@ -16,9 +16,7 @@
 package builder
 
 import (
-	"os"
 	"reflect"
-	"strconv"
 	"time"
 
 	"github.com/arduino/arduino-cli/arduino/sketch"
@@ -27,6 +25,7 @@ import (
 	"github.com/arduino/arduino-cli/legacy/builder/types"
 	"github.com/arduino/arduino-cli/legacy/builder/utils"
 	"github.com/pkg/errors"
+	"github.com/sirupsen/logrus"
 )
 
 var tr = i18n.Tr
@@ -57,31 +56,31 @@ func (s *Builder) Run(ctx *types.Context) error {
 
 		&ContainerMergeCopySketchFiles{},
 
-		utils.LogIfVerbose("info", tr("Detecting libraries used...")),
+		utils.LogIfVerbose(false, tr("Detecting libraries used...")),
 		&ContainerFindIncludes{},
 
 		&WarnAboutArchIncompatibleLibraries{},
 
-		utils.LogIfVerbose("info", tr("Generating function prototypes...")),
+		utils.LogIfVerbose(false, tr("Generating function prototypes...")),
 		&PreprocessSketch{},
 
-		utils.LogIfVerbose("info", tr("Compiling sketch...")),
+		utils.LogIfVerbose(false, tr("Compiling sketch...")),
 		&RecipeByPrefixSuffixRunner{Prefix: "recipe.hooks.sketch.prebuild", Suffix: ".pattern"},
 		&phases.SketchBuilder{},
 		&RecipeByPrefixSuffixRunner{Prefix: "recipe.hooks.sketch.postbuild", Suffix: ".pattern", SkipIfOnlyUpdatingCompilationDatabase: true},
 
-		utils.LogIfVerbose("info", tr("Compiling libraries...")),
+		utils.LogIfVerbose(false, tr("Compiling libraries...")),
 		&RecipeByPrefixSuffixRunner{Prefix: "recipe.hooks.libraries.prebuild", Suffix: ".pattern"},
 		&UnusedCompiledLibrariesRemover{},
 		&phases.LibrariesBuilder{},
 		&RecipeByPrefixSuffixRunner{Prefix: "recipe.hooks.libraries.postbuild", Suffix: ".pattern", SkipIfOnlyUpdatingCompilationDatabase: true},
 
-		utils.LogIfVerbose("info", tr("Compiling core...")),
+		utils.LogIfVerbose(false, tr("Compiling core...")),
 		&RecipeByPrefixSuffixRunner{Prefix: "recipe.hooks.core.prebuild", Suffix: ".pattern"},
 		&phases.CoreBuilder{},
 		&RecipeByPrefixSuffixRunner{Prefix: "recipe.hooks.core.postbuild", Suffix: ".pattern", SkipIfOnlyUpdatingCompilationDatabase: true},
 
-		utils.LogIfVerbose("info", tr("Linking everything together...")),
+		utils.LogIfVerbose(false, tr("Linking everything together...")),
 		&RecipeByPrefixSuffixRunner{Prefix: "recipe.hooks.linking.prelink", Suffix: ".pattern"},
 		&phases.Linker{},
 		&RecipeByPrefixSuffixRunner{Prefix: "recipe.hooks.linking.postlink", Suffix: ".pattern", SkipIfOnlyUpdatingCompilationDatabase: true},
@@ -163,7 +162,7 @@ func (s *Preprocess) Run(ctx *types.Context) error {
 	}
 
 	// Output arduino-preprocessed source
-	ctx.ExecStdout.Write([]byte(ctx.Source))
+	ctx.Stdout.Write([]byte(ctx.Source))
 	return nil
 }
 
@@ -200,9 +199,7 @@ func runCommands(ctx *types.Context, commands []types.Command) error {
 }
 
 func PrintRingNameIfDebug(ctx *types.Context, command types.Command) {
-	if ctx.DebugLevel >= 10 {
-		ctx.GetLogger().Fprintln(os.Stdout, "debug", "Ts: {0} - Running: {1}", strconv.FormatInt(time.Now().Unix(), 10), reflect.Indirect(reflect.ValueOf(command)).Type().Name())
-	}
+	logrus.Debugf("Ts: %d - Running: %s", time.Now().Unix(), reflect.Indirect(reflect.ValueOf(command)).Type().Name())
 }
 
 func RunBuilder(ctx *types.Context) error {