Skip to content

chore: Improve addOptions implementation #3998

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
gmlewis merged 3 commits into from
Feb 13, 2026
+12 −75
Merged

chore: Improve addOptions implementation #3998

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
451dd70
refactor: Improve addOptions implementation
alexandear Feb 12, 2026
47e07d3
Add test for query parameters
alexandear Feb 13, 2026
0f4fdc7
check for nil opts
alexandear Feb 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion github/actions_cache.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
8 changes: 4 additions & 4 deletions github/github.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import (
"io"
"net/http"
"net/url"
"reflect"
"regexp"
"strconv"
"strings"
Expand Down Expand Up @@ -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
}

Expand Down
34 changes: 0 additions & 34 deletions github/github_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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)
Expand Down
43 changes: 7 additions & 36 deletions github/scim_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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": [
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -115,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
})
}
Expand Down Expand Up @@ -444,40 +449,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{}, `{
Expand Down
Loading