Bbkane/parse2

CHANGELOG.md

@@ -10,7 +10,31 @@ below this description) is likely unreleased.
 
 ## Changed
 
-- Moved `SetBy` into the `Value` interface (`value.UpdatedBy()` - this allows `Flag` to be readonly and and makes the coupling between setting the value and updating `UpdatedBy` explicit
+- Moved `SetBy` into the `Value` interface (`value.UpdatedBy()` - this allows
+`Flag` to be readonly and and makes the coupling between setting the value and
+updating `UpdatedBy` explicit
+- Flags must now be the last things passed - i.e. `<appname> <section>
+<command> <flag>...`. In addition, the only flag allowed after a `<section>` is
+the `--help` flag (unfortunately the `--color` flag is NOT currently allowed to
+be passed for section help). This simplifies the parsing and will help with tab
+completion, and after that's implemented I might try to restore the old
+behavior if I get too annoyed with this limitation. Temporarily, the old
+behavior can be restored by setting the `WARG_PRE_V0_0_26_PARSE_ALGORITHM`
+environment variable, but I plan to remove that in the next version.
+
+Examples:
+
+```
+$ go run ./examples/butler --color false -h
+Parse err: Parse args error: expecting section or command, got --color
+exit status 64
+```
+
+```
+$ WARG_PRE_V0_0_26_PARSE_ALGORITHM=1 go run ./examples/butler --color false -h
+A virtual assistant
+# ... more text ...
+```
 
 # v0.0.25
 

app.go

@@ -2,6 +2,7 @@
 package warg
 
 import (
+	"errors"
 	"fmt"
 	"log"
 	"os"
@@ -259,57 +260,39 @@ func LookupMap(m map[string]string) LookupFunc {
 	}
 }
 
-type flagNameSet map[flag.Name]struct{}
-
-// addFlags adds a flag's name and alias to the set. Returns an error
-// if the name OR alias already exists
-func (fs flagNameSet) addFlags(fm flag.FlagMap) error {
-	for flagName := range fm {
-		_, exists := fs[flagName]
-		if exists {
-			return fmt.Errorf("flag or alias name exists twice: %v", flagName)
-		}
-		fs[flagName] = struct{}{}
-
-		alias := fm[flagName].Alias
-		if alias != "" {
-			_, exists := fs[alias]
-			if exists {
-				return fmt.Errorf("flag or alias name exists twice: %v", alias)
-			}
-			fs[alias] = struct{}{}
-		}
-	}
-	return nil
-}
-
-func validateFlags(
+// validateFlags2 checks that global and command flag names and aliases start with "-" and are unique.
+// It does not need to check the following scenarios:
+//
+//   - global flag names don't collide with global flag names (app will panic when adding the second global flag) - TOOD: ensure there's a test for this
+//   - command flag names in the same command don't collide with each other (app will panic when adding the second command flag) TODO: ensure there's a test for this
+//   - command flag names/aliases don't collide with command flag names/aliases in other commands (since only one command will be run, this is not a problem)
+func validateFlags2(
 	globalFlags flag.FlagMap,
 	comFlags flag.FlagMap,
 ) error {
-	nameSet := make(flagNameSet)
-	var err error
-
-	err = nameSet.addFlags(globalFlags)
-	if err != nil {
-		return err
+	nameCount := make(map[flag.Name]int)
+	for name, fl := range globalFlags {
+		nameCount[name]++
+		if fl.Alias != "" {
+			nameCount[fl.Alias]++
+		}
 	}
-
-	err = nameSet.addFlags(comFlags)
-	if err != nil {
-		return err
+	for name, fl := range comFlags {
+		nameCount[name]++
+		if fl.Alias != "" {
+			nameCount[fl.Alias]++
+		}
 	}
-
-	// fmt.Printf("%#v\n", nameSet)
-
-	for name := range nameSet {
+	var errs []error
+	for name, count := range nameCount {
 		if !strings.HasPrefix(string(name), "-") {
-			return fmt.Errorf("flag and alias names must start with '-': %#v", name)
+			errs = append(errs, fmt.Errorf("flag and alias names must start with '-': %#v", name))
+		}
+		if count > 1 {
+			errs = append(errs, fmt.Errorf("flag or alias name exists %d times: %v", count, name))
 		}
 	}
-
-	return nil
-
+	return errors.Join(errs...)
 }
 
 // Validate checks app for creation errors. It checks:
@@ -339,14 +322,35 @@ func (app *App) Validate() error {
 			return fmt.Errorf("sections must have either child sections or child commands: %#v", secName)
 		}
 
+		{
+			// child section names should not clash with child command names
+			nameCount := make(map[string]int)
+			for name := range flatSec.Sec.Commands {
+				nameCount[string(name)]++
+			}
+			for name := range flatSec.Sec.Sections {
+				nameCount[string(name)]++
+			}
+			errs := []error{}
+			for name, count := range nameCount {
+				if count > 1 {
+					errs = append(errs, fmt.Errorf("command and section name clash: %s", name))
+				}
+			}
+			err := errors.Join(errs...)
+			if err != nil {
+				return fmt.Errorf("name collision: %w", err)
+			}
+		}
+
 		for name, com := range flatSec.Sec.Commands {
 
 			// Commands must not start wtih "-"
 			if strings.HasPrefix(string(name), "-") {
 				return fmt.Errorf("command names must not start with '-': %#v", name)
 			}
 
-			err := validateFlags(app.globalFlags, com.Flags)
+			err := validateFlags2(app.globalFlags, com.Flags)
 			if err != nil {
 				return err
 			}

app_help_ext_test.go

@@ -95,6 +95,17 @@ func TestAppHelp(t *testing.T) {
 		args   []string
 		lookup warg.LookupFunc
 	}{
+		// toplevel just a toplevel help!
+		{
+			name: "toplevel",
+			app: warg.New(
+				"grabbit",
+				grabbitSection(),
+				warg.SkipValidation(),
+			),
+			args:   []string{"grabbit", "-h", "outline"},
+			lookup: warg.LookupMap(nil),
+		},
 
 		// allcommands (no command help)
 		{

app_parse.go

@@ -229,7 +229,7 @@ func resolveFlag(
 
 	// update from config
 	{
-		if canUpdate && fl.Value.UpdatedBy() == value.UpdatedByUnset && configReader != nil {
+		if canUpdate && fl.Value.UpdatedBy() == value.UpdatedByUnset && configReader != nil && fl.ConfigPath != "" {
 			fpr, err := configReader.Search(fl.ConfigPath)
 			if err != nil {
 				return err
@@ -287,7 +287,10 @@ func resolveFlag(
 	// update from default
 	{
 		if canUpdate && fl.Value.UpdatedBy() == value.UpdatedByUnset && fl.Value.HasDefault() {
-			fl.Value.ReplaceFromDefault(value.UpdatedByDefault)
+			err := fl.Value.ReplaceFromDefault(value.UpdatedByDefault)
+			if err != nil {
+				return fmt.Errorf("error updating flag %v from default: %w", name, err)
+			}
 		}
 	}
 
@@ -390,7 +393,6 @@ func NewParseOptHolder(opts ...ParseOpt) ParseOptHolder {
 }
 
 func (app *App) parseWithOptHolder(parseOptHolder ParseOptHolder) (*ParseResult, error) {
-
 	osArgs := parseOptHolder.Args
 	osLookupEnv := parseOptHolder.LookupFunc
 
@@ -540,7 +542,7 @@ func (app *App) parseWithOptHolder(parseOptHolder ParseOptHolder) (*ParseResult,
 					return &pr, nil
 				}
 			}
-			return nil, fmt.Errorf("some problem with section help: info: %v", helpInfo)
+			return nil, fmt.Errorf("some problem with command help: info: %v", helpInfo)
 		} else {
 
 			pr := ParseResult{
@@ -566,5 +568,8 @@ func (app *App) parseWithOptHolder(parseOptHolder ParseOptHolder) (*ParseResult,
 func (app *App) Parse(opts ...ParseOpt) (*ParseResult, error) {
 
 	parseOptHolder := NewParseOptHolder(opts...)
-	return app.parseWithOptHolder(parseOptHolder)
+	if _, exists := os.LookupEnv("WARG_PRE_V0_0_26_PARSE_ALGORITHM"); exists {
+		return app.parseWithOptHolder(parseOptHolder)
+	}
+	return app.parseWithOptHolder2(parseOptHolder)
 }