Skip to content

Commit

Permalink
Fix 'double print bug' for unrecognized flags
Browse files Browse the repository at this point in the history
We moved to using our own flagset in the previous commit, which lets us
then instantiate it with 'ContinueOnError', and catch the parsing error
ourselves and print it once, rather than being stuck with the pflag bug
that results in the error being double printed
(spf13/pflag#352).
  • Loading branch information
amterp committed Dec 28, 2024
1 parent 1228c2a commit 9deb170
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 14 deletions.
13 changes: 7 additions & 6 deletions core/runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ func NewRadRunner(runnerInput RunnerInput) *RadRunner {

func (r *RadRunner) Run() error {
// don't fail on unknown flags. they may be intended for the script, which we won't have parsed initially
RFlagSet = pflag.NewFlagSet(os.Args[0], pflag.ExitOnError)
RFlagSet = pflag.NewFlagSet(os.Args[0], pflag.ContinueOnError)
RFlagSet.ParseErrorsWhitelist.UnknownFlags = true

RFlagSet.Usage = func() {
Expand Down Expand Up @@ -105,13 +105,13 @@ func (r *RadRunner) Run() error {
// help not explicitly invoked, so let's try parsing other args

// re-enable erroring on unknown flags. note: maybe remove for 'catchall' args?
// todo if unknown flag passed, pflag handles the error & prints a kinda ugly msg (twice, bug).
// continue allowing unknown flags and then detect ourselves?
RFlagSet.ParseErrorsWhitelist.UnknownFlags = false

// todo apparently this is not recommended, I should be using flagsets? i.e. create new one? I THINK I DO, FOR TESTS?
// RAD-67 will prevent double-error print (see https://github.com/spf13/pflag/issues/352)
RFlagSet.Parse(os.Args[1:])
// technically re-using the flagset is apparently discouraged, but i've yet to see where it goes wrong
err = RFlagSet.Parse(os.Args[1:])
if err != nil {
RP.UsageErrorExit(err.Error())
}

posArgsIndex := 0
if FlagStdinScriptName.Value == "" {
Expand Down Expand Up @@ -196,6 +196,7 @@ func (r *RadRunner) createRslArgsFromScript() []RslArg {

return flags
}

func readSource(scriptPath string) string {
source, err := os.ReadFile(scriptPath)
if err != nil {
Expand Down
13 changes: 5 additions & 8 deletions core/testing/args_test.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package testing

import (
"fmt"
"testing"
)

Expand Down Expand Up @@ -414,22 +413,20 @@ Script args:
resetTestState()
}

// todo RAD-67 - pflag currently ExitsOnError, I think that's why this test doesn't work
func TestArgs_InvalidFlagPrintsUsageAndReturnsError(t *testing.T) {
t.Skip("TODO: RAD-67")
rsl := `
args:
name string
age int
`
fmt.Println("hi")
setupAndRunCode(t, rsl, "alice", "2", "-s", "--NO-COLOR")
expected := `Usage:
test <name> <age>
expected := `unknown shorthand flag: 's' in -s
Usage:
test <name> <age>
Script args:
--name string
--age int
--name string
--age int
` + globalFlagHelp
assertError(t, 1, expected)
Expand Down

0 comments on commit 9deb170

Please sign in to comment.