chore: Improve addOptions implementation

[alexandear]

This PR refactors addOptions to accept only a struct pointer or nil as the opts parameter. Calling addOptions(u, "") is now rejected at compile time (see TestAddOptions_QueryValues). It was unnecessary anyway and is not used in the codebase.

TestListSCIMProvisionedIdentitiesOptions_addOptions calls testAddURLOptions to verify that addOptions works properly. However, this is redundant and inconsistent with other unit tests, so it can be replaced with TestSCIMService_ListSCIMProvisionedIdentities.

We now use generics instead of reflect. But this doesn't improve performance; it remains the same.

Benchmarks

func addOptionsReflect(s string, opts any) (string, error) {
	v := reflect.ValueOf(opts)
	if v.Kind() == reflect.Pointer && v.IsNil() {
		return s, nil
	}

	u, err := url.Parse(s)
	if err != nil {
		return s, err
	}

	qs, err := query.Values(opts)
	if err != nil {
		return s, err
	}

	u.RawQuery = qs.Encode()
	return u.String(), nil
}

func BenchmarkAddOptionsReflect(b *testing.B) {
	b.Run("nil", func(b *testing.B) {
		for i := 0; i < b.N; i++ {
			opts := (*IssueListOptions)(nil)
			_, _ = addOptionsReflect("example.com", opts)
		}
	})
	b.Run("single field", func(b *testing.B) {
		for i := 0; i < b.N; i++ {
			opts := &IssueListOptions{State: "open"}
			_, _ = addOptionsReflect("example.com", opts)
		}
	})
	b.Run("all fields", func(b *testing.B) {
		for i := 0; i < b.N; i++ {
			opts := &IssueListOptions{
				Filter:    "assigned",
				State:     "open",
				Labels:    []string{"bug", "feature"},
				Sort:      "created",
				Direction: "desc",
				Since:     time.Date(2023, time.January, 1, 0, 0, 0, 0, time.UTC),
				ListOptions: ListOptions{
					Page:    1,
					PerPage: 30,
				},
			}
			_, _ = addOptionsReflect("example.com", opts)
		}
	})
}

func BenchmarkAddOptions(b *testing.B) {
	b.Run("nil", func(b *testing.B) {
		for i := 0; i < b.N; i++ {
			opts := (*IssueListOptions)(nil)
			_, _ = addOptions("example.com", opts)
		}
	})
	b.Run("single field", func(b *testing.B) {
		for i := 0; i < b.N; i++ {
			opts := &IssueListOptions{State: "open"}
			_, _ = addOptions("example.com", opts)
		}
	})
	b.Run("all fields", func(b *testing.B) {
		for i := 0; i < b.N; i++ {
			opts := &IssueListOptions{
				Filter:    "assigned",
				State:     "open",
				Labels:    []string{"bug", "feature"},
				Sort:      "created",
				Direction: "desc",
				Since:     time.Date(2023, time.January, 1, 0, 0, 0, 0, time.UTC),
				ListOptions: ListOptions{
					Page:    1,
					PerPage: 30,
				},
			}
			_, _ = addOptions("example.com", opts)
		}
	})
}

$ go test -bench=BenchmarkAddOptions -benchmem ./...
goos: darwin
goarch: arm64
pkg: github.com/google/go-github/v82/github
cpu: Apple M4 Pro
BenchmarkAddOptionsReflect/nil-12               838793499                1.470 ns/op           0 B/op            0 allocs/op
BenchmarkAddOptionsReflect/single_field-12                948915              1118 ns/op     1192 B/op          26 allocs/op
BenchmarkAddOptionsReflect/all_fields-12                  461889              2626 ns/op     2136 B/op          58 allocs/op
BenchmarkAddOptions/nil-12                              1000000000               0.9612 ns/op            0 B/op          0 allocs/op
BenchmarkAddOptions/single_field-12                      1000000              1124 ns/op     1192 B/op          26 allocs/op
BenchmarkAddOptions/all_fields-12                         455990              2692 ns/op     2136 B/op          58 allocs/op
PASS
ok      github.com/google/go-github/v82/github  9.817s
?       github.com/google/go-github/v82/test/fields     [no test files]
?       github.com/google/go-github/v82/test/integration        [no test files]

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.51%. Comparing base (ab5ad47) to head (0f4fdc7).
⚠️ Report is 7 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3998      +/-   ##
==========================================
- Coverage   93.52%   93.51%   -0.02%     
==========================================
  Files         207      207              
  Lines       17597    17596       -1     
==========================================
- Hits        16458    16455       -3     
- Misses        938      939       +1     
- Partials      201      202       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[gmlewis]

Thank you, @alexandear!
LGTM.
Awaiting second LGTM+Approval from any other contributor to this repo before merging.

cc: @stevehipwell - @zyfy29 - @Not-Dhananjay-Mishra

[stevehipwell]

The code change looks good. Is there a reason for the removal of the tests with no replacement?

[alexandear]

The code change looks good. Is there a reason for the removal of the tests with no replacement?

My bad. Thanks. I thought that TestSCIMService_ListSCIMProvisionedIdentities covers query parameters, but it's not. Extended the test; please review.

[stevehipwell]

LGTM

[gmlewis]

Thank you, @stevehipwell!
Merging.