Repository: alecthomas/kong
Pull request #85 (2020-06-01 12:30:08)

Fix sep:"none" and mapsep:"none"
According to the README.md, a value of `"none"` for the `sep` tag (for
slices) and `mapsep` (for maps) is meant to disable the separator that
would allow multiple entries to be added from a single argument.
However, using `"none"` just set the separator to the rune `'n'`.

Fix the parsing of the tag, and also update `SplitEscaped` to not split
with a separator of `-1`.

Add a new test for the new cases.

camh-: I also noticed that there is a `JoinEscaped` function that is unused. I can add the removal of that as well to this PR if you like. `SplitEscaped` is no longer really reversable with the `-1` separator.
camh-: I've added another commit as I noticed that the help placeholders we not honouring the `sep` and `mapsep` struct tags. Fixed that.
camh-: I've pushed a further commit that refactors the code a bit more but I'm happy to drop that if it does not go in a direction you want it to. Normally I would fold that into the base commit, but if you're going to squash merge anyway I've kept it separate to be easier to drop. If you want to preserve commits in history, let me know and I'll squash it in a rebase and force push the result.

Patch:
diff --git a/mapper.go b/mapper.go
index d038cb0..8a1944d 100644
--- a/mapper.go
+++ b/mapper.go
@@ -636,6 +636,9 @@ func urlMapper() MapperFunc {
 //
 //     SplitEscaped(`hello\,there,bob`, ',') == []string{"hello,there", "bob"}
 func SplitEscaped(s string, sep rune) (out []string) {
+	if sep == -1 {
+		return []string{s}
+	}
 	escaped := false
 	token := ""
 	for _, ch := range s {
diff --git a/model.go b/model.go
index 8023dce..bd9eec4 100644
--- a/model.go
+++ b/model.go
@@ -372,8 +372,8 @@ func (f *Flag) String() string {
 // FormatPlaceHolder formats the placeholder string for a Flag.
 func (f *Flag) FormatPlaceHolder() string {
 	tail := ""
-	if f.Value.IsSlice() {
-		tail += ",..."
+	if f.Value.IsSlice() && f.Value.Tag.Sep != -1 {
+		tail += string(f.Value.Tag.Sep) + "..."
 	}
 	if f.Default != "" {
 		if f.Value.Target.Kind() == reflect.String {
@@ -385,7 +385,10 @@ func (f *Flag) FormatPlaceHolder() string {
 		return f.PlaceHolder + tail
 	}
 	if f.Value.IsMap() {
-		return "KEY=VALUE;..."
+		if f.Value.Tag.MapSep != -1 {
+			tail = string(f.Value.Tag.MapSep) + "..."
+		}
+		return "KEY=VALUE" + tail
 	}
 	return strings.ToUpper(f.Name) + tail
 }
diff --git a/tag.go b/tag.go
index 88380fe..4c58517 100644
--- a/tag.go
+++ b/tag.go
@@ -151,26 +151,12 @@ func parseTag(fv reflect.Value, ft reflect.StructField) *Tag {
 	t.Short, _ = t.GetRune("short")
 	t.Hidden = t.Has("hidden")
 	t.Format = t.Get("format")
-	t.Sep, _ = t.GetRune("sep")
-	t.MapSep, _ = t.GetRune("mapsep")
+	t.Sep, _ = t.GetSep("sep", ',')
+	t.MapSep, _ = t.GetSep("mapsep", ';')
 	t.Group = t.Get("group")
 	t.Xor = t.Get("xor")
 	t.Prefix = t.Get("prefix")
 	t.Embed = t.Has("embed")
-	if t.Sep == 0 {
-		if t.Get("sep") == "none" {
-			t.Sep = -1
-		} else {
-			t.Sep = ','
-		}
-	}
-	if t.MapSep == 0 {
-		if t.Get("mapsep") == "none" {
-			t.MapSep = -1
-		} else {
-			t.MapSep = ';'
-		}
-	}
 	t.Vars = Vars{}
 	for _, set := range t.GetAll("set") {
 		parts := strings.SplitN(set, "=", 2)
@@ -232,3 +218,23 @@ func (t *Tag) GetRune(k string) (rune, error) {
 	}
 	return r, nil
 }
+
+// GetSep parses the given tag as a rune separator, allowing for a default or none.
+// The separator is returned, or -1 if "none" is specified. If the tag value is an
+// invalid utf8 sequence, the default rune is returned as well as an error. If the
+// tag value is more than one rune, the first rune is returned as well as an error.
+func (t *Tag) GetSep(k string, dflt rune) (rune, error) {
+	tv := t.Get(k)
+	if tv == "none" {
+		return -1, nil
+	} else if tv == "" {
+		return dflt, nil
+	}
+	r, size := utf8.DecodeRuneInString(tv)
+	if r == utf8.RuneError {
+		return dflt, fmt.Errorf(`%v:"%v" has a rune error`, k, tv)
+	} else if size != len(tv) {
+		return r, fmt.Errorf(`%v:"%v" is more than a single rune`, k, tv)
+	}
+	return r, nil
+}


Test patch:
diff --git a/mapper_test.go b/mapper_test.go
index 14f06d5..b6d7737 100644
--- a/mapper_test.go
+++ b/mapper_test.go
@@ -130,6 +130,7 @@ func TestDurationMapper(t *testing.T) {
 func TestSplitEscaped(t *testing.T) {
 	require.Equal(t, []string{"a", "b"}, kong.SplitEscaped("a,b", ','))
 	require.Equal(t, []string{"a,b", "c"}, kong.SplitEscaped(`a\,b,c`, ','))
+	require.Equal(t, []string{"a,b,c"}, kong.SplitEscaped(`a,b,c`, -1))
 }
 
 func TestJoinEscaped(t *testing.T) {
@@ -172,6 +173,18 @@ func TestMapWithDifferentSeparator(t *testing.T) {
 	require.Equal(t, map[string]string{"a": "b", "c": "d"}, cli.Value)
 }
 
+func TestMapWithNoSeparator(t *testing.T) {
+	var cli struct {
+		Slice []string          `sep:"none"`
+		Value map[string]string `mapsep:"none"`
+	}
+	k := mustNew(t, &cli)
+	_, err := k.Parse([]string{"--slice=a,n,c", "--value=a=b;n=d"})
+	require.NoError(t, err)
+	require.Equal(t, []string{"a,n,c"}, cli.Slice)
+	require.Equal(t, map[string]string{"a": "b;n=d"}, cli.Value)
+}
+
 func TestURLMapper(t *testing.T) {
 	var cli struct {
 		URL *url.URL `arg:""`
diff --git a/model_test.go b/model_test.go
index b128aaf..9910704 100644
--- a/model_test.go
+++ b/model_test.go
@@ -25,3 +25,44 @@ func TestModelApplicationCommands(t *testing.T) {
 	}
 	require.Equal(t, []string{"one two", "one three <four>"}, actual)
 }
+
+func TestFormatPlaceHolder(t *testing.T) {
+	var cli struct {
+		String                  string
+		DefaultInt              int    `default:"42"`
+		DefaultStr              string `default:"hello"`
+		Placeholder             string `placeholder:"world"`
+		SliceSep                []string
+		SliceNoSep              []string `sep:"none"`
+		SliceDefault            []string `default:"hello"`
+		SlicePlaceholder        []string `placeholder:"world"`
+		SliceDefaultPlaceholder []string `default:"hello" placeholder:"world"`
+		MapSep                  map[string]string
+		MapNoSep                map[string]string `mapsep:"none"`
+		MapDefault              map[string]string `mapsep:"none" default:"hello"`
+		MapPlaceholder          map[string]string `mapsep:"none" placeholder:"world"`
+	}
+	tests := map[string]string{
+		"help":                      "HELP",
+		"string":                    "STRING",
+		"default-int":               "42",
+		"default-str":               `"hello"`,
+		"placeholder":               "world",
+		"slice-sep":                 "SLICE-SEP,...",
+		"slice-no-sep":              "SLICE-NO-SEP",
+		"slice-default":             "hello,...",
+		"slice-placeholder":         "world,...",
+		"slice-default-placeholder": "hello,...",
+		"map-sep":                   "KEY=VALUE;...",
+		"map-no-sep":                "KEY=VALUE",
+		"map-default":               "hello",
+		"map-placeholder":           "world",
+	}
+
+	p := mustNew(t, &cli)
+	for _, flag := range p.Model.Flags {
+		want, ok := tests[flag.Name]
+		require.Truef(t, ok, "unknown flag name: %s", flag.Name)
+		require.Equal(t, want, flag.FormatPlaceHolder())
+	}
+}