Merge pull request #5 from github/filter-labels

refactor(label-filters): improve the label filtering

Author: GrantBirki

internal/cmd/inputs.go

@@ -3,42 +3,15 @@ package cmd
 import (
 	"errors"
 	"fmt"
+	"slices"
 )
 
+var errLabelsConflict = errors.New("--ignore-labels contains a value which conflicts with --labels")
+
 // validateInputs checks if the provided inputs are valid
 func ValidateInputs(args []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")
-	}
-
-	// 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)
-			}
-		}
-	}
-
-	// 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)
-			}
-		}
-	}
-
-	// 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)
-				}
-			}
-		}
+	if err := ValidateLabels(selectLabels, ignoreLabels); err != nil {
+		return err
 	}
 
 	// If no args and no file, we can't proceed
@@ -48,9 +21,20 @@ func ValidateInputs(args []string) error {
 
 	// Warn if no filtering options are provided at all
 	if branchPrefix == "" && branchSuffix == "" && branchRegex == "" &&
-		ignoreLabel == "" && len(ignoreLabels) == 0 && selectLabel == "" && len(selectLabels) == 0 &&
+		len(ignoreLabels) == 0 && len(selectLabels) == 0 &&
 		!requireCI && !mustBeApproved {
-		Logger.Warn("No filtering options specified. This will attempt to combine ALL open pull requests. Use --label, --labels, --ignore-label, --ignore-labels, --branch-prefix, --branch-suffix, --branch-regex, --dependabot, etc to filter.")
+		Logger.Warn("No filtering options specified. This will attempt to combine ALL open pull requests. Use  --labels, --ignore-labels, --branch-prefix, --branch-suffix, --branch-regex, --dependabot, etc to filter.")
+	}
+
+	return nil
+}
+
+func ValidateLabels(selectLabels []string, ignoreLabels []string) error {
+	// Check for conflicts between ignoreLabels and selectLabels
+	for _, ignoreL := range ignoreLabels {
+		if i := slices.Index(selectLabels, ignoreL); i != -1 {
+			return fmt.Errorf("%w: %q %q", errLabelsConflict, selectLabels[i], ignoreLabels)
+		}
 	}
 
 	return nil

internal/cmd/inputs_test.go

@@ -1,13 +1,53 @@
 package cmd
 
 import (
-	"bytes"
-	"log/slog"
+	"errors"
 	"os"
-	"strings"
 	"testing"
 )
 
+func TestValidateLabels(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		selectLabels []string
+		ignoreLabels []string
+		want         error
+	}{
+		{
+			want: nil,
+		},
+		{
+			selectLabels: []string{"a"},
+			ignoreLabels: []string{"b"},
+			want:         nil,
+		},
+		{
+			selectLabels: []string{"a"},
+			ignoreLabels: []string{"a", "b"},
+			want:         errLabelsConflict,
+		},
+		{
+			selectLabels: []string{"a", "b"},
+			ignoreLabels: []string{"b"},
+			want:         errLabelsConflict,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run("", func(t *testing.T) {
+			t.Parallel()
+
+			got := ValidateLabels(test.selectLabels, test.ignoreLabels)
+
+			if !errors.Is(got, test.want) {
+				t.Fatalf("want %s, but go %s", test.want, got)
+			}
+		})
+	}
+}
+
+/*
 // mockLogger creates a test logger that writes to a bytes.Buffer
 func setupMockLogger() (*bytes.Buffer, func()) {
 	var buf bytes.Buffer
@@ -279,6 +319,7 @@ func TestValidateInputs_ValidInputs(t *testing.T) {
 		})
 	}
 }
+*/
 
 // TestMain sets up and tears down the testing environment
 func TestMain(m *testing.M) {

internal/cmd/match_criteria.go

@@ -4,21 +4,22 @@ import (
 	"context"
 	"fmt"
 	"regexp"
+	"slices"
 	"strings"
 
 	"github.com/cli/go-gh/v2/pkg/api"
 	graphql "github.com/cli/shurcooL-graphql"
 )
 
 // checks if a PR matches all filtering criteria
-func PrMatchesCriteria(branch string, prLabels []struct{ Name string }) bool {
+func PrMatchesCriteria(branch string, prLabels []string) bool {
 	// Check branch criteria if any are specified
 	if !branchMatchesCriteria(branch) {
 		return false
 	}
 
 	// Check label criteria if any are specified
-	if !labelsMatchCriteria(prLabels) {
+	if !labelsMatch(prLabels, ignoreLabels, selectLabels) {
 		return false
 	}
 
@@ -58,64 +59,25 @@ func branchMatchesCriteria(branch string) bool {
 	return true
 }
 
-// labelsMatchCriteria checks if PR labels match the label filtering criteria
-func labelsMatchCriteria(prLabels []struct{ Name string }) bool {
-	// If no label filters are specified, all PRs pass this check
-	if ignoreLabel == "" && len(ignoreLabels) == 0 &&
-		selectLabel == "" && len(selectLabels) == 0 {
-		return true
-	}
-
-	// Check for ignore label (singular)
-	if ignoreLabel != "" {
-		for _, label := range prLabels {
-			if label.Name == ignoreLabel {
-				return false
-			}
-		}
-	}
-
-	// Check for ignore labels (plural)
-	if len(ignoreLabels) > 0 {
-		for _, ignoreL := range ignoreLabels {
-			for _, prLabel := range prLabels {
-				if prLabel.Name == ignoreL {
-					return false
-				}
-			}
+func labelsMatch(prLabels, ignoreLabels, selectLabels []string) bool {
+	for _, l := range ignoreLabels {
+		if i := slices.Index(prLabels, l); i != -1 {
+			return false
 		}
 	}
 
-	// Check for select label (singular)
-	if selectLabel != "" {
+	for _, l := range selectLabels {
 		found := false
-		for _, label := range prLabels {
-			if label.Name == selectLabel {
-				found = true
-				break
-			}
+		if i := slices.Index(prLabels, l); i != -1 {
+			found = true
+			break
 		}
+
 		if !found {
 			return false
 		}
 	}
 
-	// Check for select labels (plural)
-	if len(selectLabels) > 0 {
-		for _, requiredLabel := range selectLabels {
-			found := false
-			for _, prLabel := range prLabels {
-				if prLabel.Name == requiredLabel {
-					found = true
-					break
-				}
-			}
-			if !found {
-				return false
-			}
-		}
-	}
-
 	return true
 }
 

internal/cmd/match_criteria_test.go

@@ -0,0 +1,59 @@
+package cmd
+
+import "testing"
+
+func TestLabelsMatch(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		prLabels     []string
+		ignoreLabels []string
+		selectLabels []string
+		want         bool
+	}{
+		{
+			want: true,
+		},
+
+		{
+			prLabels:     []string{"a", "b"},
+			ignoreLabels: []string{"b"},
+			want:         false,
+		},
+		{
+			prLabels:     []string{"a", "b"},
+			ignoreLabels: []string{"b", "c"},
+			want:         false,
+		},
+
+		{
+			prLabels:     []string{"a"},
+			ignoreLabels: []string{"b"},
+			selectLabels: []string{"c"},
+			want:         false,
+		},
+		{
+			prLabels:     []string{"a", "c"},
+			ignoreLabels: []string{"b"},
+			selectLabels: []string{"c"},
+			want:         true,
+		},
+		{
+			prLabels:     []string{"a"},
+			ignoreLabels: []string{"b"},
+			selectLabels: []string{"a", "c"},
+			want:         true,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run("", func(t *testing.T) {
+			t.Parallel()
+
+			got := labelsMatch(test.prLabels, test.ignoreLabels, test.selectLabels)
+			if got != test.want {
+				t.Errorf("want %v, got %v", test.want, got)
+			}
+		})
+	}
+}

internal/cmd/root.go

@@ -14,19 +14,20 @@ import (
 )
 
 var (
-	branchPrefix        string
-	branchSuffix        string
-	branchRegex         string
-	selectLabel         string
-	selectLabels        []string
-	addLabels           []string
-	addAssignees        []string
+	branchPrefix string
+	branchSuffix string
+	branchRegex  string
+
+	selectLabels []string
+	ignoreLabels []string
+
+	addLabels    []string
+	addAssignees []string
+
 	requireCI           bool
 	mustBeApproved      bool
 	autoclose           bool
 	updateBranch        bool
-	ignoreLabel         string
-	ignoreLabels        []string
 	reposFile           string
 	minimum             int
 	defaultOwner        string
@@ -46,7 +47,7 @@ func NewRootCmd() *cobra.Command {
 	  # Note: You should use some form of filtering to avoid combining all open PRs in a repository.
 	  # For example, you can filter by branch name, labels, or other criteria.
 	  # Forms of filtering include:
-	  # --label, --labels, --ignore-label, --ignore-labels, --branch-prefix, --branch-suffix, --branch-regex, --dependabot, etc.
+	  # --labels, --ignore-labels, --branch-prefix, --branch-suffix, --branch-regex, --dependabot, etc.
 
       # Basic usage with a single repository to combine all pull requests into one
       gh combine owner/repo
@@ -75,11 +76,11 @@ func NewRootCmd() *cobra.Command {
       gh combine owner/repo --branch-regex "dependabot/.*"
     
       # Filter PRs by labels
-      gh combine owner/repo --label dependencies        # PRs must have this single label
+      gh combine owner/repo --labels dependencies           # PRs must have this single label
       gh combine owner/repo --labels security,dependencies  # PRs must have ALL these labels
       
       # Exclude PRs by labels
-      gh combine owner/repo --ignore-label wip          # Ignore PRs with this label
+      gh combine owner/repo --ignore-labels wip         # Ignore PRs with this label
       gh combine owner/repo --ignore-labels wip,draft   # Ignore PRs with ANY of these labels
     
       # Set requirements for PRs to be combined
@@ -105,12 +106,7 @@ func NewRootCmd() *cobra.Command {
 	rootCmd.Flags().StringVar(&branchSuffix, "branch-suffix", "", "Branch suffix to filter PRs")
 	rootCmd.Flags().StringVar(&branchRegex, "branch-regex", "", "Regex pattern to filter PRs by branch name")
 
-	// Label selection flags - singular and plural forms
-	rootCmd.Flags().StringVar(&selectLabel, "label", "", "Only include PRs with this specific label")
 	rootCmd.Flags().StringSliceVar(&selectLabels, "labels", nil, "Only include PRs with ALL these labels (comma-separated)")
-
-	// Label ignoring flags - singular and plural forms
-	rootCmd.Flags().StringVar(&ignoreLabel, "ignore-label", "", "Ignore PRs with this specific label")
 	rootCmd.Flags().StringSliceVar(&ignoreLabels, "ignore-labels", nil, "Ignore PRs with ANY of these labels (comma-separated)")
 
 	// Labels to add to the combined PR
@@ -278,8 +274,16 @@ func processRepository(ctx context.Context, client *api.RESTClient, graphQlClien
 	for _, pull := range pulls {
 		branch := pull.Head.Ref
 
+		// Temporary workaround because passing structures is useless in this
+		// context.
+		// Eventually the []Labels should have better support.
+		labels := []string{}
+		for _, label := range pull.Labels {
+			labels = append(labels, label.Name)
+		}
+
 		// Check if PR matches all filtering criteria
-		if !PrMatchesCriteria(branch, pull.Labels) {
+		if !PrMatchesCriteria(branch, labels) {
 			continue
 		}