From 451dd704a3ce52adb92de7e0cf6ceb517b3a5e15 Mon Sep 17 00:00:00 2001 From: Oleksandr Redko Date: Thu, 12 Feb 2026 15:16:12 +0200 Subject: [PATCH 1/3] refactor: Improve addOptions implementation --- github/actions_cache.go | 2 +- github/github.go | 8 ++++---- github/github_test.go | 34 ---------------------------------- github/scim_test.go | 34 ---------------------------------- 4 files changed, 5 insertions(+), 73 deletions(-) diff --git a/github/actions_cache.go b/github/actions_cache.go index 852e9860f1c..c883effb900 100644 --- a/github/actions_cache.go +++ b/github/actions_cache.go @@ -114,7 +114,7 @@ func (s *ActionsService) ListCaches(ctx context.Context, owner, repo string, opt //meta:operation DELETE /repos/{owner}/{repo}/actions/caches func (s *ActionsService) DeleteCachesByKey(ctx context.Context, owner, repo, key string, ref *string) (*Response, error) { u := fmt.Sprintf("repos/%v/%v/actions/caches", owner, repo) - u, err := addOptions(u, ActionsCache{Key: &key, Ref: ref}) + u, err := addOptions(u, &ActionsCache{Key: &key, Ref: ref}) if err != nil { return nil, err } diff --git a/github/github.go b/github/github.go index 0ebeaee96be..4abaa7c01b1 100644 --- a/github/github.go +++ b/github/github.go @@ -19,7 +19,6 @@ import ( "io" "net/http" "net/url" - "reflect" "regexp" "strconv" "strings" @@ -312,11 +311,12 @@ type RawOptions struct { Type RawType } +type structPtr[T any] interface{ *T } + // addOptions adds the parameters in opts as URL query parameters to s. opts // must be a struct whose fields may contain "url" tags. -func addOptions(s string, opts any) (string, error) { - v := reflect.ValueOf(opts) - if v.Kind() == reflect.Pointer && v.IsNil() { +func addOptions[P structPtr[T], T any](s string, opts P) (string, error) { + if opts == nil { return s, nil } diff --git a/github/github_test.go b/github/github_test.go index e9409acdec3..a43eb7764a7 100644 --- a/github/github_test.go +++ b/github/github_test.go @@ -187,33 +187,6 @@ func testJSONMarshal(t *testing.T, v any, want string) { } } -// Test whether the v fields have the url tag and the parsing of v -// produces query parameters that corresponds to the want string. -func testAddURLOptions(t *testing.T, url string, v any, want string) { - t.Helper() - - vt := reflect.Indirect(reflect.ValueOf(v)).Type() - for i := range vt.NumField() { - field := vt.Field(i) - if alias, ok := field.Tag.Lookup("url"); ok { - if alias == "" { - t.Errorf("The field %+v has a blank url tag", field) - } - } else { - t.Errorf("The field %+v has no url tag specified", field) - } - } - - got, err := addOptions(url, v) - if err != nil { - t.Errorf("Unable to add %#v as query parameters", v) - } - - if got != want { - t.Errorf("addOptions(%q, %#v) returned %v, want %v", url, v, got, want) - } -} - // Test how bad options are handled. Method f under test should // return an error. func testBadOptions(t *testing.T, methodName string, f func() error) { @@ -3055,13 +3028,6 @@ func TestAbuseRateLimitError(t *testing.T) { } } -func TestAddOptions_QueryValues(t *testing.T) { - t.Parallel() - if _, err := addOptions("yo", ""); err == nil { - t.Error("addOptions err = nil, want error") - } -} - func TestBareDo_returnsOpenBody(t *testing.T) { t.Parallel() client, mux, _ := setup(t) diff --git a/github/scim_test.go b/github/scim_test.go index bde81493b27..162de74f518 100644 --- a/github/scim_test.go +++ b/github/scim_test.go @@ -444,40 +444,6 @@ func TestUpdateAttributeForSCIMUserOptions_Marshal(t *testing.T) { testJSONMarshal(t, u, want) } -func TestListSCIMProvisionedIdentitiesOptions_addOptions(t *testing.T) { - t.Parallel() - testJSONMarshal(t, &ListSCIMProvisionedIdentitiesOptions{}, `{ - "StartIndex": null, - "Count": null, - "Filter": null - }`) - - url := "some/path" - - testAddURLOptions(t, url, &ListSCIMProvisionedIdentitiesOptions{}, url) - - testAddURLOptions( - t, - url, - &ListSCIMProvisionedIdentitiesOptions{ - StartIndex: Ptr(1), - Count: Ptr(10), - }, - fmt.Sprintf("%v?count=10&startIndex=1", url), - ) - - testAddURLOptions( - t, - url, - &ListSCIMProvisionedIdentitiesOptions{ - StartIndex: Ptr(1), - Count: Ptr(10), - Filter: Ptr("test"), - }, - fmt.Sprintf("%v?count=10&filter=test&startIndex=1", url), - ) -} - func TestSCIMUserName_Marshal(t *testing.T) { t.Parallel() testJSONMarshal(t, &SCIMUserName{}, `{ From 47e07d3965fc4b2fbe7c85f00a6b51dff7c5d1b3 Mon Sep 17 00:00:00 2001 From: Oleksandr Redko Date: Fri, 13 Feb 2026 12:45:03 +0200 Subject: [PATCH 2/3] Add test for query parameters --- github/scim_test.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/github/scim_test.go b/github/scim_test.go index 162de74f518..19b7ad9f89b 100644 --- a/github/scim_test.go +++ b/github/scim_test.go @@ -20,6 +20,7 @@ func TestSCIMService_ListSCIMProvisionedIdentities(t *testing.T) { mux.HandleFunc("/scim/v2/organizations/o/Users", func(w http.ResponseWriter, r *http.Request) { testMethod(t, r, "GET") + testFormValues(t, r, values{"startIndex": "1", "count": "10", "filter": `userName="Octocat"`}) w.WriteHeader(http.StatusOK) _, _ = w.Write([]byte(`{ "schemas": [ @@ -61,7 +62,11 @@ func TestSCIMService_ListSCIMProvisionedIdentities(t *testing.T) { }) ctx := t.Context() - opts := &ListSCIMProvisionedIdentitiesOptions{} + opts := &ListSCIMProvisionedIdentitiesOptions{ + StartIndex: Ptr(1), + Count: Ptr(10), + Filter: Ptr(`userName="Octocat"`), + } identities, _, err := client.SCIM.ListSCIMProvisionedIdentities(ctx, "o", opts) if err != nil { t.Errorf("SCIM.ListSCIMProvisionedIdentities returned error: %v", err) From 0f4fdc7667f794bfcfd4f09fad2d69a3390084cb Mon Sep 17 00:00:00 2001 From: Oleksandr Redko Date: Fri, 13 Feb 2026 12:49:00 +0200 Subject: [PATCH 3/3] check for nil opts --- github/scim_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/github/scim_test.go b/github/scim_test.go index 19b7ad9f89b..3b092427ccd 100644 --- a/github/scim_test.go +++ b/github/scim_test.go @@ -120,7 +120,7 @@ func TestSCIMService_ListSCIMProvisionedIdentities(t *testing.T) { }) testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) { - _, r, err := client.SCIM.ListSCIMProvisionedIdentities(ctx, "o", opts) + _, r, err := client.SCIM.ListSCIMProvisionedIdentities(ctx, "o", nil) return r, err }) }