From e8fa61f44b5ef2284b258638f7221d4712b21645 Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Wed, 26 Feb 2025 14:30:20 -0800 Subject: [PATCH 1/2] Separate partitioning from globbing in cmdutil/args package and consumers In the previous commit, GlobPaths was overloaded, containing logic specific to command use-cases. This commit removes that functionality from GlobPaths and back into the commands that have the special use-cases. To do this, I've introduced a new Partition util in cmdutil/args.go that will separate a slice into two slices given a predicate. This functionality is leveraged by both the special use-cases described above to separate the command-specific syntax from the globable filepaths. --- pkg/cmd/gist/create/create.go | 9 +++- pkg/cmd/gist/create/create_test.go | 2 +- pkg/cmd/release/shared/upload.go | 9 +++- pkg/cmdutil/args.go | 43 +++++++++++----- pkg/cmdutil/args_test.go | 80 ++++++++++++++++++++++++------ 5 files changed, 111 insertions(+), 32 deletions(-) diff --git a/pkg/cmd/gist/create/create.go b/pkg/cmd/gist/create/create.go index e212a08f355..48e9a1d47a4 100644 --- a/pkg/cmd/gist/create/create.go +++ b/pkg/cmd/gist/create/create.go @@ -105,11 +105,18 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co } func createRun(opts *CreateOptions) error { - filenames, err := cmdutil.GlobPaths(opts.Filenames, nil) + + readFromStdInArg, filenames := cmdutil.Partition(opts.Filenames, func(f string) bool { + return f == "-" + }) + + filenames, err := cmdutil.GlobPaths(filenames) if err != nil { return err } + filenames = append(filenames, readFromStdInArg...) + if len(filenames) == 0 { filenames = []string{"-"} } diff --git a/pkg/cmd/gist/create/create_test.go b/pkg/cmd/gist/create/create_test.go index 237486eb3c5..c2105ee0e35 100644 --- a/pkg/cmd/gist/create/create_test.go +++ b/pkg/cmd/gist/create/create_test.go @@ -225,7 +225,7 @@ func Test_createRun(t *testing.T) { responseStatus: http.StatusOK, }, { - name: "multiple files", + name: "when both a file and the stdin '-' are provided, it matches on all the files", opts: &CreateOptions{ Filenames: []string{fixtureFile, "-"}, }, diff --git a/pkg/cmd/release/shared/upload.go b/pkg/cmd/release/shared/upload.go index be30c010b10..ab7533320e8 100644 --- a/pkg/cmd/release/shared/upload.go +++ b/pkg/cmd/release/shared/upload.go @@ -37,12 +37,17 @@ type AssetForUpload struct { } func AssetsFromArgs(args []string) (assets []*AssetForUpload, err error) { - args, err = cmdutil.GlobPaths(args, func(pattern string) bool { - return strings.Contains(pattern, "#") + labeledArgs, unlabeledArgs := cmdutil.Partition(args, func(arg string) bool { + return strings.Contains(arg, "#") }) + + args, err = cmdutil.GlobPaths(unlabeledArgs) if err != nil { return nil, err } + + args = append(args, labeledArgs...) + for _, arg := range args { var label string fn := arg diff --git a/pkg/cmdutil/args.go b/pkg/cmdutil/args.go index 5f8b0d6e5e0..b0a22e9b5fc 100644 --- a/pkg/cmdutil/args.go +++ b/pkg/cmdutil/args.go @@ -59,22 +59,41 @@ func NoArgsQuoteReminder(cmd *cobra.Command, args []string) error { return FlagErrorf("%s", errMsg) } -func GlobPaths(patterns []string, exclude func(pattern string) bool) ([]string, error) { +// Partition takes a slice of any type T and separates it into two slices +// of the same type based on the provided predicate function. Any item +// that returns true for the predicate will be included in the first slice +// returned, and any item that returns false for the predicate will be +// included in the second slice returned. +func Partition[T any](slice []T, predicate func(T) bool) ([]T, []T) { + var matching, nonMatching []T + for _, item := range slice { + if predicate(item) { + matching = append(matching, item) + } else { + nonMatching = append(nonMatching, item) + } + } + return matching, nonMatching +} + +// GlobPaths expands a list of file patterns into a list of file paths. +// If no files match a pattern, that pattern will return an error. +// If no pattern is passed, this returns an empty list and no error. +// +// For information on supported glob patterns, see +// https://pkg.go.dev/path/filepath#Match +func GlobPaths(patterns []string) ([]string, error) { expansions := []string{} for _, pattern := range patterns { - if pattern == "-" || (exclude != nil && exclude(pattern)) { - expansions = append(expansions, pattern) - } else { - matches, err := filepath.Glob(pattern) - if err != nil { - return nil, fmt.Errorf("%s: %v", pattern, err) - } - if len(matches) == 0 { - return []string{}, fmt.Errorf("no matches found for `%s`", pattern) - } - expansions = append(expansions, matches...) + matches, err := filepath.Glob(pattern) + if err != nil { + return nil, fmt.Errorf("%s: %v", pattern, err) + } + if len(matches) == 0 { + return []string{}, fmt.Errorf("no matches found for `%s`", pattern) } + expansions = append(expansions, matches...) } return expansions, nil diff --git a/pkg/cmdutil/args_test.go b/pkg/cmdutil/args_test.go index bdb7e70edcf..4e880dd273f 100644 --- a/pkg/cmdutil/args_test.go +++ b/pkg/cmdutil/args_test.go @@ -4,7 +4,6 @@ import ( "errors" "os" "path/filepath" - "strings" "testing" "github.com/stretchr/testify/assert" @@ -58,6 +57,69 @@ func TestMinimumNs_with_error(t *testing.T) { } } +func TestPartition(t *testing.T) { + tests := []struct { + name string + slice []any + predicate func(any) bool + wantMatching []any + wantNonMatching []any + }{ + { + name: "When the slice is empty, it returns two empty slices", + slice: []any{}, + predicate: func(any) bool { + return true + }, + wantMatching: []any{}, + wantNonMatching: []any{}, + }, + { + name: "when the slice has one element that satisfies the predicate, it returns a slice with that element and an empty slice", + slice: []any{ + "foo", + }, + predicate: func(any) bool { + return true + }, + wantMatching: []any{"foo"}, + wantNonMatching: []any{}, + }, + { + name: "when the slice has one element that does not satisfy the predicate, it returns an empty slice and a slice with that element", + slice: []any{ + "foo", + }, + predicate: func(any) bool { + return false + }, + wantMatching: []any{}, + wantNonMatching: []any{"foo"}, + }, + { + name: "when the slice has multiple elements, it returns a slice with the elements that satisfy the predicate and a slice with the elements that do not satisfy the predicate", + slice: []any{ + "foo", + "bar", + "baz", + }, + predicate: func(s any) bool { + return s.(string) != "foo" + }, + wantMatching: []any{"bar", "baz"}, + wantNonMatching: []any{"foo"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotMatching, gotNonMatching := Partition(tt.slice, tt.predicate) + assert.ElementsMatch(t, tt.wantMatching, gotMatching) + assert.ElementsMatch(t, tt.wantNonMatching, gotNonMatching) + }) + } +} + func TestGlobPaths(t *testing.T) { tests := []struct { name string @@ -72,18 +134,6 @@ func TestGlobPaths(t *testing.T) { wantOut: []string{}, wantErr: nil, }, - { - name: "When - is passed, return -", - patterns: []string{"-"}, - wantOut: []string{"-"}, - wantErr: nil, - }, - { - name: "When labels are passed, return labels", - patterns: []string{"file.txt#Text File", "README.md#README"}, - wantOut: []string{"file.txt#Text File", "README.md#README"}, - wantErr: nil, - }, { name: "When no files match, it returns an empty expansions array with error", patterns: []string{"foo"}, @@ -131,9 +181,7 @@ func TestGlobPaths(t *testing.T) { cleanupFn := createTestDir(t) defer cleanupFn() - got, err := GlobPaths(tt.patterns, func(pattern string) bool { - return strings.Contains(pattern, "#") - }) + got, err := GlobPaths(tt.patterns) if tt.wantErr != nil { assert.EqualError(t, err, tt.wantErr.Error()) } else { From 064a305e781b55d25284f70a61d1f63e3a489fe9 Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Wed, 26 Feb 2025 14:43:40 -0800 Subject: [PATCH 2/2] Add test to validate that the order of '-' in gh gist create args doesn't matter --- pkg/cmd/gist/create/create_test.go | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/gist/create/create_test.go b/pkg/cmd/gist/create/create_test.go index c2105ee0e35..44f0ba284ef 100644 --- a/pkg/cmd/gist/create/create_test.go +++ b/pkg/cmd/gist/create/create_test.go @@ -225,7 +225,7 @@ func Test_createRun(t *testing.T) { responseStatus: http.StatusOK, }, { - name: "when both a file and the stdin '-' are provided, it matches on all the files", + name: "when both a file and the stdin '-' are provided, it matches on all the files passed in and stdin", opts: &CreateOptions{ Filenames: []string{fixtureFile, "-"}, }, @@ -248,6 +248,30 @@ func Test_createRun(t *testing.T) { }, responseStatus: http.StatusOK, }, + { + name: "when both a file and the stdin '-' are provided, but '-' is not the last argument, it matches on all the files provided and stdin", + opts: &CreateOptions{ + Filenames: []string{"-", fixtureFile}, + }, + stdin: "cool stdin content", + wantOut: "https://gist.github.com/aa5a315d61ae9438b18d\n", + wantStderr: "- Creating gist with multiple files\n✓ Created secret gist fixture.txt\n", + wantErr: false, + wantParams: map[string]interface{}{ + "description": "", + "updated_at": "0001-01-01T00:00:00Z", + "public": false, + "files": map[string]interface{}{ + "fixture.txt": map[string]interface{}{ + "content": "{}", + }, + "gistfile1.txt": map[string]interface{}{ + "content": "cool stdin content", + }, + }, + }, + responseStatus: http.StatusOK, + }, { name: "file with empty content", opts: &CreateOptions{