From 048471f217e2b75386b10f43de71addc6559b344 Mon Sep 17 00:00:00 2001
From: Alexander Terp <alexander.terp@gmail.com>
Date: Sat, 21 Dec 2024 16:08:57 +1100
Subject: [PATCH] Bug fix: return non-0 exit code on pflag parse error

We are currently doing this

  pflag.Usage = func() {
    r.RunUsageExit()
  }

That usage gets invoked if pflag fails to parse (e.g. due to an unknown
flag), but then rather than letting pflag exit with a non-0 error code,
the RunUsageExit() func exits with a 0 code, misleading callers into
thinking we had a successful script invocation.

That's a bug. We seem to be able to safely just remove the 'exit' part
to fix it and still have everything else working properly.

I took this opportunity to investigate the 'double unknown error' print
issue a bit. It's a bug in the pflag lib, I'm not the only one to have
found it: https://github.com/spf13/pflag/issues/352

Unfortunately pflag is not maintained anymore (last release >5 years
ago), so very small chance this is ever getting fixed.

The only solution (bar forking pflag, but I don't think this is a
legitimate reason to) is to switch to using our own flagset so that we
can control the error handling to be ContinueOnError instead of
ExitOnError. This way, we can then avoid the double print and handle the
error ourselves. Anyway, that's a future ticket.
---
 core/runner.go            |  3 +-
 core/testing/args_test.go | 65 ++++++++++++++++++++++++++++++++++++++-
 2 files changed, 66 insertions(+), 2 deletions(-)

diff --git a/core/runner.go b/core/runner.go
index ac9618b..3089a24 100644
--- a/core/runner.go
+++ b/core/runner.go
@@ -24,7 +24,7 @@ func (r *RadRunner) Run() error {
 	pflag.CommandLine.ParseErrorsWhitelist.UnknownFlags = true
 
 	pflag.Usage = func() {
-		r.RunUsageExit()
+		r.RunUsage()
 	}
 
 	r.globalFlags = CreateAndRegisterGlobalFlags()
@@ -106,6 +106,7 @@ func (r *RadRunner) Run() error {
 	pflag.CommandLine.ParseErrorsWhitelist.UnknownFlags = false
 
 	// todo apparently this is not recommended, I should be using flagsets? I THINK I DO, FOR TESTS?
+	//  RAD-67 will prevent double-error print (see https://github.com/spf13/pflag/issues/352)
 	pflag.Parse()
 
 	posArgsIndex := 0
diff --git a/core/testing/args_test.go b/core/testing/args_test.go
index 64c01ed..2dd20d5 100644
--- a/core/testing/args_test.go
+++ b/core/testing/args_test.go
@@ -1,6 +1,8 @@
 package testing
 
-import "testing"
+import (
+	"testing"
+)
 
 const (
 	setupArgRsl = `
@@ -312,3 +314,64 @@ print(name, isTall)
 	assertOnlyOutput(t, stdOutBuffer, expected)
 	resetTestState()
 }
+
+func TestArgs_MissingArgsPrintsUsageAndReturnsError(t *testing.T) {
+	rsl := `
+args:
+	name string
+	age int
+`
+	setupAndRunCode(t, rsl, "alice", "--NO-COLOR")
+	expected := `Missing required arguments: [age]
+Usage:
+  test <name> <age>
+
+Script args:
+      --name string   
+      --age int       
+
+` + globalFlagHelp
+	assertError(t, 1, expected)
+	resetTestState()
+}
+
+func TestArgs_TooManyArgsPrintsUsageAndReturnsError(t *testing.T) {
+	rsl := `
+args:
+	name string
+	age int
+`
+	setupAndRunCode(t, rsl, "alice", "2", "3", "--NO-COLOR")
+	expected := `Too many positional arguments. Unused: [3]
+Usage:
+  test <name> <age>
+
+Script args:
+      --name string   
+      --age int       
+
+` + globalFlagHelp
+	assertError(t, 1, expected)
+	resetTestState()
+}
+
+// todo RAD-67 - pflag currently ExitsOnError, I think that's why this test doesn't work
+//func TestArgs_InvalidFlagPrintsUsageAndReturnsError(t *testing.T) {
+//	rsl := `
+//args:
+//	name string
+//	age int
+//`
+//	fmt.Println("hi")
+//	setupAndRunCode(t, rsl, "alice", "2", "-s", "--NO-COLOR")
+//	expected := `Usage:
+//  test <name> <age>
+//
+//Script args:
+//      --name string
+//      --age int
+//
+//` + globalFlagHelp
+//	assertError(t, 1, expected)
+//	resetTestState()
+//}