Merge pull request #96 from zimmski/quoting-quoting-everywhere
do quoting in INI and Help
diff --git a/convert.go b/convert.go
index be8de39..d5675c5 100644
--- a/convert.go
+++ b/convert.go
@@ -294,6 +294,32 @@
return nil
}
+func isPrint(s string) bool {
+ for _, c := range s {
+ if !strconv.IsPrint(c) {
+ return false
+ }
+ }
+
+ return true
+}
+
+func quoteIfNeeded(s string) string {
+ if !isPrint(s) {
+ return strconv.Quote(s)
+ }
+
+ return s
+}
+
+func unquoteIfPossible(s string) (string, error) {
+ if s[0] != '"' {
+ return s, nil
+ }
+
+ return strconv.Unquote(s)
+}
+
func wrapText(s string, l int, prefix string) string {
// Basic text wrapping of s at spaces to fit in l
var ret string
diff --git a/help.go b/help.go
index d16f3b1..e26fcd0 100644
--- a/help.go
+++ b/help.go
@@ -171,7 +171,13 @@
def, _ = convertToString(option.value, option.tag)
}
} else if len(defs) != 0 {
- def = strings.Join(defs, ", ")
+ l := len(defs) - 1
+
+ for i := 0; i < l; i++ {
+ def += quoteIfNeeded(defs[i]) + ", "
+ }
+
+ def += quoteIfNeeded(defs[l])
}
var envDef string
@@ -188,8 +194,7 @@
var desc string
if def != "" {
- desc = fmt.Sprintf("%s (%v)%s", option.Description, def,
- envDef)
+ desc = fmt.Sprintf("%s (%v)%s", option.Description, def, envDef)
} else {
desc = option.Description + envDef
}
diff --git a/help_test.go b/help_test.go
index def3043..32220fb 100644
--- a/help_test.go
+++ b/help_test.go
@@ -15,8 +15,8 @@
PtrSlice []*string `long:"ptrslice" description:"A slice of pointers to string"`
EmptyDescription bool `long:"empty-description"`
- Default string `long:"default" default:"Some value" description:"Test default value"`
- DefaultArray []string `long:"default-array" default:"Some value" default:"Another value" description:"Test default array value"`
+ Default string `long:"default" default:"Some\nvalue" description:"Test default value"`
+ DefaultArray []string `long:"default-array" default:"Some value" default:"Other\tvalue" description:"Test default array value"`
DefaultMap map[string]string `long:"default-map" default:"some:value" default:"another:value" description:"Testdefault map value"`
EnvDefault1 string `long:"env-default1" default:"Some value" env:"ENV_DEFAULT" description:"Test env-default1 value"`
EnvDefault2 string `long:"env-default2" env:"ENV_DEFAULT" description:"Test env-default2 value"`
@@ -79,8 +79,8 @@
/c: Call phone number
/ptrslice: A slice of pointers to string
/empty-description
- /default: Test default value (Some value)
- /default-array: Test default array value (Some value, Another value)
+ /default: Test default value ("Some\nvalue")
+ /default-array: Test default array value (Some value, "Other\tvalue")
/default-map: Testdefault map value (some:value, another:value)
/env-default1: Test env-default1 value (Some value) [%ENV_DEFAULT%]
/env-default2: Test env-default2 value [%ENV_DEFAULT%]
@@ -115,8 +115,8 @@
-c= Call phone number
--ptrslice= A slice of pointers to string
--empty-description
- --default= Test default value (Some value)
- --default-array= Test default array value (Some value, Another value)
+ --default= Test default value ("Some\nvalue")
+ --default-array= Test default array value (Some value, "Other\tvalue")
--default-map= Testdefault map value (some:value, another:value)
--env-default1= Test env-default1 value (Some value) [$ENV_DEFAULT]
--env-default2= Test env-default2 value [$ENV_DEFAULT]
diff --git a/ini_private.go b/ini_private.go
index 2e72b87..0f2bc06 100644
--- a/ini_private.go
+++ b/ini_private.go
@@ -7,16 +7,22 @@
"os"
"reflect"
"sort"
+ "strconv"
"strings"
)
type iniValue struct {
- Name string
- Value string
+ Name string
+ Value string
+ Quoted bool
+ LineNumber uint
}
type iniSection []iniValue
-type ini map[string]iniSection
+type ini struct {
+ File string
+ Sections map[string]iniSection
+}
func readFullLine(reader *bufio.Reader) (string, error) {
var line []byte
@@ -102,50 +108,49 @@
oname := optionIniName(option)
- commentOption := ""
- if (options&(IniIncludeDefaults|IniCommentDefaults)) == IniIncludeDefaults|IniCommentDefaults && option.valueIsDefault() {
- commentOption = "; "
- }
+ commentOption := (options&(IniIncludeDefaults|IniCommentDefaults)) == IniIncludeDefaults|IniCommentDefaults && option.valueIsDefault()
- switch val.Type().Kind() {
+ kind := val.Type().Kind()
+ switch kind {
case reflect.Slice:
- for idx := 0; idx < val.Len(); idx++ {
- v, _ := convertToString(val.Index(idx), option.tag)
- fmt.Fprintf(writer, "%s%s = %s\n", commentOption, oname, v)
- }
+ kind = val.Type().Elem().Kind()
if val.Len() == 0 {
- fmt.Fprintf(writer, "; %s =\n", oname)
+ writeOption(writer, oname, kind, "", "", true, option.iniQuote)
+ } else {
+ for idx := 0; idx < val.Len(); idx++ {
+ v, _ := convertToString(val.Index(idx), option.tag)
+
+ writeOption(writer, oname, kind, "", v, commentOption, option.iniQuote)
+ }
}
case reflect.Map:
- mkeys := val.MapKeys()
- keys := make([]string, len(val.MapKeys()))
- kkmap := make(map[string]reflect.Value)
-
- for i, k := range mkeys {
- keys[i], _ = convertToString(k, option.tag)
- kkmap[keys[i]] = k
- }
-
- sort.Strings(keys)
-
- for _, k := range keys {
- v, _ := convertToString(val.MapIndex(kkmap[k]), option.tag)
-
- fmt.Fprintf(writer, "%s%s = %s:%s\n", commentOption, oname, k, v)
- }
+ kind = val.Type().Elem().Kind()
if val.Len() == 0 {
- fmt.Fprintf(writer, "; %s =\n", oname)
+ writeOption(writer, oname, kind, "", "", true, option.iniQuote)
+ } else {
+ mkeys := val.MapKeys()
+ keys := make([]string, len(val.MapKeys()))
+ kkmap := make(map[string]reflect.Value)
+
+ for i, k := range mkeys {
+ keys[i], _ = convertToString(k, option.tag)
+ kkmap[keys[i]] = k
+ }
+
+ sort.Strings(keys)
+
+ for _, k := range keys {
+ v, _ := convertToString(val.MapIndex(kkmap[k]), option.tag)
+
+ writeOption(writer, oname, kind, k, v, commentOption, option.iniQuote)
+ }
}
default:
v, _ := convertToString(val, option.tag)
- if len(v) != 0 {
- fmt.Fprintf(writer, "%s%s = %s\n", commentOption, oname, v)
- } else {
- fmt.Fprintf(writer, "%s%s =\n", commentOption, oname)
- }
+ writeOption(writer, oname, kind, "", v, commentOption, option.iniQuote)
}
if comments {
@@ -158,6 +163,27 @@
}
}
+func writeOption(writer io.Writer, optionName string, optionType reflect.Kind, optionKey string, optionValue string, commentOption bool, forceQuote bool) {
+ if forceQuote || (optionType == reflect.String && !isPrint(optionValue)) {
+ optionValue = strconv.Quote(optionValue)
+ }
+
+ comment := ""
+ if commentOption {
+ comment = "; "
+ }
+
+ fmt.Fprintf(writer, "%s%s =", comment, optionName)
+
+ if optionKey != "" {
+ fmt.Fprintf(writer, " %s:%s", optionKey, optionValue)
+ } else if optionValue != "" {
+ fmt.Fprintf(writer, " %s", optionValue)
+ }
+
+ fmt.Fprintln(writer)
+}
+
func writeCommandIni(command *Command, namespace string, writer io.Writer, options IniOptions) {
command.eachGroup(func(group *Group) {
writeGroupIni(command, group, namespace, writer, options)
@@ -194,7 +220,7 @@
return nil
}
-func readIniFromFile(filename string) (ini, error) {
+func readIniFromFile(filename string) (*ini, error) {
file, err := os.Open(filename)
if err != nil {
@@ -206,8 +232,11 @@
return readIni(file, filename)
}
-func readIni(contents io.Reader, filename string) (ini, error) {
- ret := make(ini)
+func readIni(contents io.Reader, filename string) (*ini, error) {
+ ret := &ini{
+ File: filename,
+ Sections: make(map[string]iniSection),
+ }
reader := bufio.NewReader(contents)
@@ -215,7 +244,7 @@
section := make(iniSection, 0, 10)
sectionname := ""
- ret[sectionname] = section
+ ret.Sections[sectionname] = section
var lineno uint
@@ -256,11 +285,11 @@
}
sectionname = name
- section = ret[name]
+ section = ret.Sections[name]
if section == nil {
section = make(iniSection, 0, 10)
- ret[name] = section
+ ret.Sections[name] = section
}
continue
@@ -279,13 +308,30 @@
name := strings.TrimSpace(keyval[0])
value := strings.TrimSpace(keyval[1])
+ quoted := false
+
+ if len(value) != 0 && value[0] == '"' {
+ if v, err := strconv.Unquote(value); err == nil {
+ value = v
+
+ quoted = true
+ } else {
+ return nil, &IniError{
+ Message: err.Error(),
+ File: filename,
+ LineNumber: lineno,
+ }
+ }
+ }
section = append(section, iniValue{
- Name: name,
- Value: value,
+ Name: name,
+ Value: value,
+ Quoted: quoted,
+ LineNumber: lineno,
})
- ret[sectionname] = section
+ ret.Sections[sectionname] = section
}
return ret, nil
@@ -311,10 +357,12 @@
return nil
}
-func (i *IniParser) parse(ini ini) error {
+func (i *IniParser) parse(ini *ini) error {
p := i.parser
- for name, section := range ini {
+ var quotesLookup = make(map[*Option]bool)
+
+ for name, section := range ini.Sections {
groups := i.matchingGroups(name)
if len(groups) == 0 {
@@ -343,10 +391,11 @@
if opt == nil {
if (p.Options & IgnoreUnknown) == None {
- return newError(
- ErrUnknownFlag,
- fmt.Sprintf("unknown option: %s", inival.Name),
- )
+ return &IniError{
+ Message: fmt.Sprintf("unknown option: %s", inival.Name),
+ File: ini.File,
+ LineNumber: inival.LineNumber,
+ }
}
continue
@@ -356,15 +405,51 @@
if !opt.canArgument() && len(inival.Value) == 0 {
pval = nil
+ } else {
+ if opt.value.Type().Kind() == reflect.Map {
+ parts := strings.SplitN(inival.Value, ":", 2)
+
+ // only handle unquoting
+ if len(parts) == 2 && parts[1][0] == '"' {
+ if v, err := strconv.Unquote(parts[1]); err == nil {
+ parts[1] = v
+
+ inival.Quoted = true
+ } else {
+ return &IniError{
+ Message: err.Error(),
+ File: ini.File,
+ LineNumber: inival.LineNumber,
+ }
+ }
+
+ s := parts[0] + ":" + parts[1]
+
+ pval = &s
+ }
+ }
}
if err := opt.set(pval); err != nil {
- return wrapError(err)
+ return &IniError{
+ Message: err.Error(),
+ File: ini.File,
+ LineNumber: inival.LineNumber,
+ }
+ }
+
+ // either all INI values are quoted or only values who need quoting
+ if _, ok := quotesLookup[opt]; !inival.Quoted || !ok {
+ quotesLookup[opt] = inival.Quoted
}
opt.tag.Set("_read-ini-name", inival.Name)
}
}
+ for opt, quoted := range quotesLookup {
+ opt.iniQuote = quoted
+ }
+
return nil
}
diff --git a/ini_test.go b/ini_test.go
index e2b5cdc..8c3176c 100644
--- a/ini_test.go
+++ b/ini_test.go
@@ -5,6 +5,7 @@
"fmt"
"io/ioutil"
"os"
+ "reflect"
"strings"
"testing"
)
@@ -54,11 +55,11 @@
EmptyDescription = false
; Test default value
-Default = Some value
+Default = "Some\nvalue"
; Test default array value
DefaultArray = Some value
-DefaultArray = Another value
+DefaultArray = "Other\tvalue"
; Testdefault map value
DefaultMap = another:value
@@ -109,11 +110,11 @@
; EmptyDescription = false
; Test default value
-; Default = Some value
+; Default = "Some\nvalue"
; Test default array value
; DefaultArray = Some value
-; DefaultArray = Another value
+; DefaultArray = "Other\tvalue"
; Testdefault map value
; DefaultMap = another:value
@@ -242,16 +243,23 @@
verbose = true
verbose = true
+DefaultMap = another:"value\n1"
+DefaultMap = some:value 2
+
[Application Options]
; A slice of pointers to string
; PtrSlice =
; Test default value
-Default = Some value
+Default = "New\nvalue"
+
+; Test env-default1 value
+EnvDefault1 = New value
[Other Options]
# A slice of strings
-# StringSlice =
+StringSlice = "some\nvalue"
+StringSlice = another value
; A map from string to int
int-map = a:2
@@ -268,6 +276,16 @@
assertBoolArray(t, opts.Verbose, []bool{true, true})
+ if v := map[string]string{"another": "value\n1", "some": "value 2"}; !reflect.DeepEqual(opts.DefaultMap, v) {
+ t.Fatalf("Expected %#v for DefaultMap but got %#v", v, opts.DefaultMap)
+ }
+
+ assertString(t, opts.Default, "New\nvalue")
+
+ assertString(t, opts.EnvDefault1, "New value")
+
+ assertStringArray(t, opts.Other.StringSlice, []string{"some\nvalue", "another value"})
+
if v, ok := opts.Other.IntMap["a"]; !ok {
t.Errorf("Expected \"a\" in Other.IntMap")
} else if v != 2 {
@@ -281,6 +299,222 @@
}
}
+func TestReadAndWriteIni(t *testing.T) {
+ var tests = []struct {
+ options IniOptions
+ read string
+ write string
+ }{
+ {
+ IniIncludeComments,
+ `[Application Options]
+; Show verbose debug information
+verbose = true
+verbose = true
+
+; Test default value
+Default = "quote me"
+
+; Test default array value
+DefaultArray = 1
+DefaultArray = "2"
+DefaultArray = 3
+
+; Testdefault map value
+; DefaultMap =
+
+; Test env-default1 value
+EnvDefault1 = env-def
+
+; Test env-default2 value
+EnvDefault2 = env-def
+
+[Other Options]
+; A slice of strings
+; StringSlice =
+
+; A map from string to int
+int-map = a:2
+int-map = b:"3"
+
+`,
+ `[Application Options]
+; Show verbose debug information
+verbose = true
+verbose = true
+
+; Test default value
+Default = "quote me"
+
+; Test default array value
+DefaultArray = 1
+DefaultArray = 2
+DefaultArray = 3
+
+; Testdefault map value
+; DefaultMap =
+
+; Test env-default1 value
+EnvDefault1 = env-def
+
+; Test env-default2 value
+EnvDefault2 = env-def
+
+[Other Options]
+; A slice of strings
+; StringSlice =
+
+; A map from string to int
+int-map = a:2
+int-map = b:3
+
+`,
+ },
+ {
+ IniIncludeComments,
+ `[Application Options]
+; Show verbose debug information
+verbose = true
+verbose = true
+
+; Test default value
+Default = "quote me"
+
+; Test default array value
+DefaultArray = "1"
+DefaultArray = "2"
+DefaultArray = "3"
+
+; Testdefault map value
+; DefaultMap =
+
+; Test env-default1 value
+EnvDefault1 = env-def
+
+; Test env-default2 value
+EnvDefault2 = env-def
+
+[Other Options]
+; A slice of strings
+; StringSlice =
+
+; A map from string to int
+int-map = a:"2"
+int-map = b:"3"
+
+`,
+ `[Application Options]
+; Show verbose debug information
+verbose = true
+verbose = true
+
+; Test default value
+Default = "quote me"
+
+; Test default array value
+DefaultArray = "1"
+DefaultArray = "2"
+DefaultArray = "3"
+
+; Testdefault map value
+; DefaultMap =
+
+; Test env-default1 value
+EnvDefault1 = env-def
+
+; Test env-default2 value
+EnvDefault2 = env-def
+
+[Other Options]
+; A slice of strings
+; StringSlice =
+
+; A map from string to int
+int-map = a:"2"
+int-map = b:"3"
+
+`,
+ },
+ }
+
+ for _, test := range tests {
+ var opts helpOptions
+
+ p := NewNamedParser("TestIni", Default)
+ p.AddGroup("Application Options", "The application options", &opts)
+
+ inip := NewIniParser(p)
+
+ read := strings.NewReader(test.read)
+ err := inip.Parse(read)
+ if err != nil {
+ t.Fatalf("Unexpected error: %s", err)
+ }
+
+ var write bytes.Buffer
+ inip.Write(&write, test.options)
+
+ got := write.String()
+
+ msg := fmt.Sprintf("with ini options %b", test.options)
+ assertDiff(t, got, test.write, msg)
+ }
+}
+
+func TestReadIniWrongQuoting(t *testing.T) {
+ var tests = []struct {
+ iniFile string
+ lineNumber uint
+ }{
+ {
+ iniFile: `Default = "New\nvalue`,
+ lineNumber: 1,
+ },
+ {
+ iniFile: `StringSlice = "New\nvalue`,
+ lineNumber: 1,
+ },
+ {
+ iniFile: `StringSlice = "New\nvalue"
+ StringSlice = "Second\nvalue`,
+ lineNumber: 2,
+ },
+ {
+ iniFile: `DefaultMap = some:"value`,
+ lineNumber: 1,
+ },
+ {
+ iniFile: `DefaultMap = some:value
+ DefaultMap = another:"value`,
+ lineNumber: 2,
+ },
+ }
+
+ for _, test := range tests {
+ var opts helpOptions
+
+ p := NewNamedParser("TestIni", Default)
+ p.AddGroup("Application Options", "The application options", &opts)
+
+ inip := NewIniParser(p)
+
+ inic := test.iniFile
+
+ b := strings.NewReader(inic)
+ err := inip.Parse(b)
+
+ if err == nil {
+ t.Fatalf("Expect error")
+ }
+
+ iniError := err.(*IniError)
+
+ if iniError.LineNumber != test.lineNumber {
+ t.Fatalf("Expect error on line %d", test.lineNumber)
+ }
+ }
+}
+
func TestIniCommands(t *testing.T) {
var opts struct {
Value string `short:"v" long:"value"`
@@ -354,7 +588,15 @@
t.Fatalf("Expected error")
}
- assertError(t, err, ErrUnknownFlag, "unknown option: value")
+ iniError := err.(*IniError)
+
+ if v := uint(2); iniError.LineNumber != v {
+ t.Errorf("Expected opts.Add.Name to be %d, but got %d", v, iniError.LineNumber)
+ }
+
+ if v := "unknown option: value"; iniError.Message != v {
+ t.Errorf("Expected opts.Add.Name to be %s, but got %s", v, iniError.Message)
+ }
}
func TestIniParse(t *testing.T) {
diff --git a/option.go b/option.go
index 26f2441..29e702c 100644
--- a/option.go
+++ b/option.go
@@ -68,9 +68,11 @@
// The struct field value which the option represents.
value reflect.Value
- iniUsedName string
- tag multiTag
- isSet bool
+ // Determines if the option will be always quoted in the INI output
+ iniQuote bool
+
+ tag multiTag
+ isSet bool
}
// LongNameWithNamespace returns the option's long name with the group namespaces
diff --git a/parser_private.go b/parser_private.go
index ee637e9..67a5b02 100644
--- a/parser_private.go
+++ b/parser_private.go
@@ -164,11 +164,22 @@
}
err = option.set(nil)
- } else if argument != nil {
- err = option.set(argument)
- } else if canarg && !s.eof() {
- arg := s.pop()
- err = option.set(&arg)
+ } else if argument != nil || (canarg && !s.eof()) {
+ var arg string
+
+ if argument != nil {
+ arg = *argument
+ } else {
+ arg = s.pop()
+ }
+
+ if option.tag.Get("unquote") != "false" {
+ arg, err = unquoteIfPossible(arg)
+ }
+
+ if err == nil {
+ err = option.set(&arg)
+ }
} else if option.OptionalArgument {
option.empty()
diff --git a/parser_test.go b/parser_test.go
index add87d8..444705d 100644
--- a/parser_test.go
+++ b/parser_test.go
@@ -3,6 +3,7 @@
import (
"os"
"reflect"
+ "strconv"
"strings"
"testing"
"time"
@@ -12,6 +13,10 @@
Int int `long:"i"`
IntDefault int `long:"id" default:"1"`
+ String string `long:"str"`
+ StringDefault string `long:"strd" default:"abc"`
+ StringNotUnquoted string `long:"strnot" unquote:"false"`
+
Time time.Duration `long:"t"`
TimeDefault time.Duration `long:"td" default:"1m"`
@@ -35,6 +40,9 @@
Int: 0,
IntDefault: 1,
+ String: "",
+ StringDefault: "abc",
+
Time: 0,
TimeDefault: time.Minute,
@@ -47,11 +55,14 @@
},
{
msg: "non-zero value arguments, expecting overwritten arguments",
- args: []string{"--i=3", "--id=3", "--t=3ms", "--td=3ms", "--m=c:3", "--md=c:3", "--s=3", "--sd=3"},
+ args: []string{"--i=3", "--id=3", "--str=def", "--strd=def", "--t=3ms", "--td=3ms", "--m=c:3", "--md=c:3", "--s=3", "--sd=3"},
expected: defaultOptions{
Int: 3,
IntDefault: 3,
+ String: "def",
+ StringDefault: "def",
+
Time: 3 * time.Millisecond,
TimeDefault: 3 * time.Millisecond,
@@ -64,11 +75,14 @@
},
{
msg: "zero value arguments, expecting overwritten arguments",
- args: []string{"--i=0", "--id=0", "--t=0ms", "--td=0s", "--m=:0", "--md=:0", "--s=0", "--sd=0"},
+ args: []string{"--i=0", "--id=0", "--str=\"\"", "--strd=\"\"", "--t=0ms", "--td=0s", "--m=:0", "--md=:0", "--s=0", "--sd=0"},
expected: defaultOptions{
Int: 0,
IntDefault: 0,
+ String: "",
+ StringDefault: "",
+
Time: 0,
TimeDefault: 0,
@@ -99,6 +113,76 @@
}
}
+func TestUnquoting(t *testing.T) {
+ var tests = []struct {
+ arg string
+ err error
+ value string
+ }{
+ {
+ arg: "\"abc",
+ err: strconv.ErrSyntax,
+ value: "",
+ },
+ {
+ arg: "\"\"abc\"",
+ err: strconv.ErrSyntax,
+ value: "",
+ },
+ {
+ arg: "\"abc\"",
+ err: nil,
+ value: "abc",
+ },
+ {
+ arg: "\"\\\"abc\\\"\"",
+ err: nil,
+ value: "\"abc\"",
+ },
+ {
+ arg: "\"\\\"abc\"",
+ err: nil,
+ value: "\"abc",
+ },
+ }
+
+ for _, test := range tests {
+ var opts defaultOptions
+
+ for _, delimiter := range []bool{false, true} {
+ p := NewParser(&opts, None)
+
+ var err error
+ if delimiter {
+ _, err = p.ParseArgs([]string{"--str=" + test.arg, "--strnot=" + test.arg})
+ } else {
+ _, err = p.ParseArgs([]string{"--str", test.arg, "--strnot", test.arg})
+ }
+
+ if test.err == nil {
+ if err != nil {
+ t.Fatalf("Expected no error but got: %v", err)
+ }
+
+ if test.value != opts.String {
+ t.Fatalf("Expected String to be %q but got %q", test.value, opts.String)
+ }
+ if q := strconv.Quote(test.value); q != opts.StringNotUnquoted {
+ t.Fatalf("Expected StringDefault to be %q but got %q", q, opts.StringNotUnquoted)
+ }
+ } else {
+ if err == nil {
+ t.Fatalf("Expected error")
+ } else if e, ok := err.(*Error); ok {
+ if strings.HasPrefix(e.Message, test.err.Error()) {
+ t.Fatalf("Expected error message to end with %q but got %v", test.err.Error(), e.Message)
+ }
+ }
+ }
+ }
+ }
+}
+
// envRestorer keeps a copy of a set of env variables and can restore the env from them
type envRestorer struct {
env map[string]string
