wip: drive by comments

internal/cmd/combine_prs.go

@@ -1,3 +1,4 @@
+// @grant: this whole code should be moved to the github package.
 package cmd
 
 import (

internal/cmd/inputs.go

@@ -3,55 +3,68 @@ package cmd
 import (
 	"errors"
 	"fmt"
+	"slices"
+)
+
+// @grant: whenever possible, use sentinel errors. This ensures that those
+// values can be testsed with `errors.Is()` correctly.
+var (
+	errNoRepo               = errors.New("must specify repositories or provide a file containing a list of repositories with --file")
+	errLabelConflict        = errors.New("--ignore-label(s) and --label(s) cannot have the same value")
+	errLabelsConflict       = errors.New("--label(s) contains a value which conflicts with --ignore-label(s)")
+	errIgnoreLabelsConflict = errors.New("--ignore-label(s) contains a value which conflicts with --label(s)")
 )
 
 // validateInputs checks if the provided inputs are valid
 func ValidateInputs(args []string) error {
+	// @grant: using global variables is terrible for tests, so here I've
+	// injected them into the function for validation.
+	if err := ValidateLabels(selectLabel, selectLabels, ignoreLabel, ignoreLabels); err != nil {
+		return err
+	}
+
+	// If no args and no file, we can't proceed
+	if len(args) == 0 && reposFile == "" {
+		return errNoRepo
+	}
+
+	// Warn if no filtering options are provided at all
+	if branchPrefix == "" && branchSuffix == "" && branchRegex == "" &&
+		ignoreLabel == "" && len(ignoreLabels) == 0 && selectLabel == "" && len(selectLabels) == 0 &&
+		!requireCI && !mustBeApproved {
+		Logger.Warn("No filtering options specified. This will attempt to combine ALL open pull requests.")
+	}
+
+	return nil
+}
+
+// @grant: this function is 100% self contained, and does the same logic as
+// before, which makes it really easier to test.
+func ValidateLabels(selectLabel string, selectLabels []string, ignoreLabel string, ignoreLabels []string) error {
 	// Check that ignore-label and select-label are not the same
 	if ignoreLabel != "" && selectLabel != "" && ignoreLabel == selectLabel {
-		return errors.New("--ignore-label(s) and --label(s) cannot have the same value")
+		return errLabelConflict
 	}
 
+	// @grant: slices.Index beats a `for` and an `if`.
 	// Check for conflicts between ignoreLabels and selectLabel
-	if selectLabel != "" && len(ignoreLabels) > 0 {
-		for _, ignoreL := range ignoreLabels {
-			if ignoreL == selectLabel {
-				return fmt.Errorf("--ignore-labels contains %q which conflicts with --label %q", ignoreL, selectLabel)
-			}
-		}
+	if i := slices.Index(ignoreLabels, selectLabel); i != -1 {
+		return fmt.Errorf("%w: %q %q", errIgnoreLabelsConflict, ignoreLabels[i], selectLabel)
 	}
 
 	// Check for conflicts between ignoreLabel and selectLabels
-	if ignoreLabel != "" && len(selectLabels) > 0 {
-		for _, selectL := range selectLabels {
-			if selectL == ignoreLabel {
-				return fmt.Errorf("--label(s) contains %q which conflicts with --ignore-label %q", selectL, ignoreLabel)
-			}
-		}
+	if i := slices.Index(selectLabels, ignoreLabel); i != -1 {
+		return fmt.Errorf("%w: %q %q", errLabelsConflict, selectLabels[i], ignoreLabel)
 	}
 
+	// @grant: no need to check for the length if we iterate directly, the
+	// performance gain is moot.
 	// Check for conflicts between ignoreLabels and selectLabels
-	if len(ignoreLabels) > 0 && len(selectLabels) > 0 {
-		for _, ignoreL := range ignoreLabels {
-			for _, selectL := range selectLabels {
-				if ignoreL == selectL {
-					return fmt.Errorf("--ignore-labels contains %q which conflicts with --labels containing the same value", ignoreL)
-				}
-			}
+	for _, ignoreL := range ignoreLabels {
+		if i := slices.Index(selectLabels, ignoreL); i != -1 {
+			return fmt.Errorf("%w: %q %q", errIgnoreLabelsConflict, ignoreLabels[i], selectLabel)
 		}
 	}
 
-	// If no args and no file, we can't proceed
-	if len(args) == 0 && reposFile == "" {
-		return errors.New("must specify repositories or provide a file containing a list of repositories with --file")
-	}
-
-	// Warn if no filtering options are provided at all
-	if branchPrefix == "" && branchSuffix == "" && branchRegex == "" &&
-		ignoreLabel == "" && len(ignoreLabels) == 0 && selectLabel == "" && len(selectLabels) == 0 &&
-		!requireCI && !mustBeApproved {
-		Logger.Warn("No filtering options specified. This will attempt to combine ALL open pull requests.")
-	}
-
 	return nil
 }

internal/cmd/inputs_test.go

@@ -2,6 +2,7 @@ package cmd
 
 import (
 	"bytes"
+	"errors"
 	"log/slog"
 	"os"
 	"strings"
@@ -19,6 +20,68 @@ func setupMockLogger() (*bytes.Buffer, func()) {
 	}
 }
 
+// @grant: this is a better pattern to write tests.
+// In a nutshell, you make each function as self-contained as possible, create
+// and iterate over a list of test cases and ensure the expected output is
+// received.
+func TestValidateLabels(t *testing.T) {
+	tests := []struct {
+		selectLabel  string
+		selectLabels []string
+		ignoreLabel  string
+		ignoreLabels []string
+		want         error
+	}{
+		{
+			// empty labels
+			want: nil,
+		},
+
+		{
+			selectLabel: "enhancement",
+			ignoreLabel: "bug",
+			want:        nil,
+		},
+
+		{
+			selectLabel: "enhancement",
+			ignoreLabel: "enhancement",
+			want:        errLabelConflict,
+		},
+
+		{
+			selectLabel:  "enhancement",
+			ignoreLabels: []string{"bug", "enhancement"},
+			want:         errIgnoreLabelsConflict,
+		},
+
+		{
+			selectLabels: []string{"bug", "enhancement"},
+			ignoreLabel:  "enhancement",
+			want:         errLabelsConflict,
+		},
+
+		{
+			selectLabels: []string{"bug", "enhancement"},
+			ignoreLabels: []string{"request", "bug"},
+			want:         errIgnoreLabelsConflict,
+		},
+
+		{
+			selectLabels: []string{"bug", "enhancement"},
+			ignoreLabels: []string{"request", "docs"},
+			want:         nil,
+		},
+	}
+
+	for _, test := range tests {
+		got := ValidateLabels(test.selectLabel, test.selectLabels, test.ignoreLabel, test.ignoreLabels)
+		if !errors.Is(got, test.want) {
+			t.Fatalf("want %v, got %v", test.want, got)
+		}
+	}
+}
+
 func TestValidateInputs_NoReposSpecified(t *testing.T) {
 	// Save original values
 	origReposFile := reposFile

internal/cmd/match_criteria.go

@@ -119,6 +119,7 @@ func labelsMatchCriteria(prLabels []struct{ Name string }) bool {
 	return true
 }
 
+// @grant: this should be moved to the github package
 // GraphQL response structure for PR status info
 type prStatusResponse struct {
 	Data struct {
@@ -142,6 +143,7 @@ type prStatusResponse struct {
 	} `json:"errors,omitempty"`
 }
 
+// @grant: this should be moved to the github package
 // GetPRStatusInfo fetches both CI status and approval status using GitHub's GraphQL API
 func GetPRStatusInfo(ctx context.Context, graphQlClient *api.GraphQLClient, owner, repo string, prNumber int) (*prStatusResponse, error) {
 	// Check for context cancellation