chore!: Add sliceofpointers custom linter (#3447)

BREAKING CHANGE: `ListOAuthApps` now returns `([]*OAuthApp, error)` instead of `([]OAuthApp, error)`.
google · Jan 22, 2025 · 07b0446 · 07b0446
1 parent 04070d2
commit 07b0446
Show file tree

Hide file tree

Showing 17 changed files with 215 additions and 40 deletions.
diff --git a/.custom-gcl.yml b/.custom-gcl.yml
@@ -0,0 +1,4 @@
+version: v1.62.0
+plugins:
+  - module: "github.com/google/go-github/v68/tools/sliceofpointers"
+    path: ./tools/sliceofpointers
diff --git a/.gitignore b/.gitignore
@@ -14,3 +14,6 @@ vendor/
 
 # goenv local version. See https://github.com/syndbg/goenv/blob/master/COMMANDS.md#goenv-local for more info.
 .go-version
+
+# golangci-lint -v custom generates the following local file:
+custom-gcl
diff --git a/.golangci.yml b/.golangci.yml
@@ -18,12 +18,18 @@ linters:
     - perfsprint
     - paralleltest
     - revive
+    - sliceofpointers
     - stylecheck
     - tparallel
     - unconvert
     - unparam
     - whitespace
 linters-settings:
+  custom:
+    sliceofpointers:
+      type: module
+      description: Reports usage of []*string and slices of structs without pointers.
+      original-url: github.com/google/go-github/v68/tools/sliceofpointers
   gocritic:
     disable-all: true
     enabled-checks:
@@ -38,7 +44,7 @@ linters-settings:
   misspell:
     locale: US
     ignore-words:
-      - "analyses"  # returned by the GitHub API
+      - "analyses" # returned by the GitHub API
       - "cancelled" # returned by the GitHub API
     # extra words from https://go.dev//wiki/Spelling
     extra-words:
@@ -102,28 +108,28 @@ issues:
       path: _test\.go
 
     # We need to pass nil Context in order to test DoBare erroring on nil ctx.
-    - linters: [ staticcheck ]
-      text: 'SA1012: do not pass a nil Context'
+    - linters: [staticcheck]
+      text: "SA1012: do not pass a nil Context"
       path: _test\.go
 
     # We need to use sha1 for validating signatures
-    - linters: [ gosec ]
-      text: 'G505: Blocklisted import crypto/sha1: weak cryptographic primitive'
+    - linters: [gosec]
+      text: "G505: Blocklisted import crypto/sha1: weak cryptographic primitive"
 
     # This is adapted from golangci-lint's default exclusions. It disables linting for error checks on
     # os.RemoveAll, fmt.Fprint*, fmt.Scanf, and any function ending in "Close".
-    - linters: [ errcheck ]
+    - linters: [errcheck]
       text: Error return value of .(.*Close|fmt\.Fprint.*|fmt\.Scanf|os\.Remove(All)?). is not checked
 
     # We don't care about file inclusion via variable in examples or internal tools.
-    - linters: [ gosec ]
-      text: 'G304: Potential file inclusion via variable'
+    - linters: [gosec]
+      text: "G304: Potential file inclusion via variable"
       path: '^(example|tools)\/'
 
     # We don't run parallel integration tests
-    - linters: [ paralleltest, tparallel ]
-      path: '^test/integration'
+    - linters: [paralleltest, tparallel]
+      path: "^test/integration"
 
     # Because fmt.Sprint(reset.Unix())) is more readable than strconv.FormatInt(reset.Unix(), 10).
-    - linters: [ perfsprint]
-      text: 'fmt.Sprint.* can be replaced with faster strconv.FormatInt'
+    - linters: [perfsprint]
+      text: "fmt.Sprint.* can be replaced with faster strconv.FormatInt"
diff --git a/github/codespaces_secrets_test.go b/github/codespaces_secrets_test.go
@@ -25,7 +25,7 @@ func TestCodespacesService_ListSecrets(t *testing.T) {
 		methodName string
 	}
 	opts := &ListOptions{Page: 2, PerPage: 2}
-	tests := []test{
+	tests := []*test{
 		{
 			name: "User",
 			handleFunc: func(mux *http.ServeMux) {
@@ -128,7 +128,7 @@ func TestCodespacesService_GetSecret(t *testing.T) {
 		badCall    func(context.Context, *Client) (*Secret, *Response, error)
 		methodName string
 	}
-	tests := []test{
+	tests := []*test{
 		{
 			name: "User",
 			handleFunc: func(mux *http.ServeMux) {
@@ -222,7 +222,7 @@ func TestCodespacesService_CreateOrUpdateSecret(t *testing.T) {
 		badCall    func(context.Context, *Client, *EncryptedSecret) (*Response, error)
 		methodName string
 	}
-	tests := []test{
+	tests := []*test{
 		{
 			name: "User",
 			handleFunc: func(mux *http.ServeMux) {
@@ -318,7 +318,7 @@ func TestCodespacesService_DeleteSecret(t *testing.T) {
 		badCall    func(context.Context, *Client) (*Response, error)
 		methodName string
 	}
-	tests := []test{
+	tests := []*test{
 		{
 			name: "User",
 			handleFunc: func(mux *http.ServeMux) {
@@ -401,7 +401,7 @@ func TestCodespacesService_GetPublicKey(t *testing.T) {
 		methodName string
 	}
 
-	tests := []test{
+	tests := []*test{
 		{
 			name: "User",
 			handleFunc: func(mux *http.ServeMux) {
@@ -496,7 +496,7 @@ func TestCodespacesService_ListSelectedReposForSecret(t *testing.T) {
 		methodName string
 	}
 	opts := &ListOptions{Page: 2, PerPage: 2}
-	tests := []test{
+	tests := []*test{
 		{
 			name: "User",
 			handleFunc: func(mux *http.ServeMux) {
@@ -581,7 +581,7 @@ func TestCodespacesService_SetSelectedReposForSecret(t *testing.T) {
 		methodName string
 	}
 	ids := SelectedRepoIDs{64780797}
-	tests := []test{
+	tests := []*test{
 		{
 			name: "User",
 			handleFunc: func(mux *http.ServeMux) {
@@ -653,7 +653,7 @@ func TestCodespacesService_AddSelectedReposForSecret(t *testing.T) {
 		methodName string
 	}
 	repo := &Repository{ID: Ptr(int64(1234))}
-	tests := []test{
+	tests := []*test{
 		{
 			name: "User",
 			handleFunc: func(mux *http.ServeMux) {
@@ -721,7 +721,7 @@ func TestCodespacesService_RemoveSelectedReposFromSecret(t *testing.T) {
 		methodName string
 	}
 	repo := &Repository{ID: Ptr(int64(1234))}
-	tests := []test{
+	tests := []*test{
 		{
 			name: "User",
 			handleFunc: func(mux *http.ServeMux) {

diff --git a/github/github.go b/github/github.go
@@ -1117,7 +1117,8 @@ GitHub API docs: https://docs.github.com/rest/#client-errors
 type ErrorResponse struct {
 	Response *http.Response `json:"-"`       // HTTP response that caused this error
 	Message  string         `json:"message"` // error message
-	Errors   []Error        `json:"errors"`  // more detail on individual errors
+	//nolint:sliceofpointers
+	Errors []Error `json:"errors"` // more detail on individual errors
 	// Block is only populated on certain types of errors such as code 451.
 	Block *ErrorBlock `json:"block,omitempty"`
 	// Most errors will also include a documentation_url field pointing

diff --git a/github/strings_test.go b/github/strings_test.go
@@ -49,6 +49,7 @@ func TestStringify(t *testing.T) {
 		{Ptr(123), `123`},
 		{Ptr(false), `false`},
 		{
+			//nolint:sliceofpointers
 			[]*string{Ptr("a"), Ptr("b")},
 			`["a" "b"]`,
 		},

diff --git a/scrape/apps.go b/scrape/apps.go
@@ -46,13 +46,13 @@ func (c *Client) AppRestrictionsEnabled(org string) (bool, error) {
 
 // ListOAuthApps lists the reviewed OAuth Applications for the
 // specified organization (whether approved or denied).
-func (c *Client) ListOAuthApps(org string) ([]OAuthApp, error) {
+func (c *Client) ListOAuthApps(org string) ([]*OAuthApp, error) {
 	doc, err := c.get("/organizations/%s/settings/oauth_application_policy", org)
 	if err != nil {
 		return nil, err
 	}
 
-	var apps []OAuthApp
+	var apps []*OAuthApp
 	doc.Find(".oauth-application-allowlist ul > li").Each(func(i int, s *goquery.Selection) {
 		var app OAuthApp
 		app.Name = s.Find(".request-info strong").First().Text()
@@ -73,7 +73,7 @@ func (c *Client) ListOAuthApps(org string) ([]OAuthApp, error) {
 		} else if r := s.Find(".request-indicator .denied-request"); r.Length() > 0 {
 			app.State = OAuthAppDenied
 		}
-		apps = append(apps, app)
+		apps = append(apps, &app)
 	})
 
 	return apps, nil

diff --git a/scrape/apps_test.go b/scrape/apps_test.go
@@ -61,7 +61,7 @@ func Test_ListOAuthApps(t *testing.T) {
 	if err != nil {
 		t.Fatalf("ListOAuthApps(e) returned err: %v", err)
 	}
-	want := []OAuthApp{
+	want := []*OAuthApp{
 		{
 			ID:          22222,
 			Name:        "Coveralls",

diff --git a/scrape/forms.go b/scrape/forms.go
@@ -35,7 +35,7 @@ type htmlForm struct {
 //
 // In the future, we might want to allow a custom selector to be passed in to
 // further restrict what forms will be returned.
-func parseForms(node *html.Node) (forms []htmlForm) {
+func parseForms(node *html.Node) (forms []*htmlForm) {
 	if node == nil {
 		return nil
 	}
@@ -71,7 +71,7 @@ func parseForms(node *html.Node) (forms []htmlForm) {
 			value := s.Text()
 			form.Values.Add(name, value)
 		})
-		forms = append(forms, form)
+		forms = append(forms, &form)
 	})
 
 	return forms

diff --git a/scrape/forms_test.go b/scrape/forms_test.go
@@ -16,22 +16,22 @@ func Test_ParseForms(t *testing.T) {
 	tests := []struct {
 		description string
 		html        string
-		forms       []htmlForm
+		forms       []*htmlForm
 	}{
 		{"no forms", `<html></html>`, nil},
-		{"empty form", `<html><form></form></html>`, []htmlForm{{Values: url.Values{}}}},
+		{"empty form", `<html><form></form></html>`, []*htmlForm{{Values: url.Values{}}}},
 		{
 			"single form with one value",
 			`<html><form action="a" method="m"><input name="n1" value="v1"></form></html>`,
-			[]htmlForm{{Action: "a", Method: "m", Values: url.Values{"n1": {"v1"}}}},
+			[]*htmlForm{{Action: "a", Method: "m", Values: url.Values{"n1": {"v1"}}}},
 		},
 		{
 			"two forms",
 			`<html>
 			  <form action="a1" method="m1"><input name="n1" value="v1"></form>
 			  <form action="a2" method="m2"><input name="n2" value="v2"></form>
 			</html>`,
-			[]htmlForm{
+			[]*htmlForm{
 				{Action: "a1", Method: "m1", Values: url.Values{"n1": {"v1"}}},
 				{Action: "a2", Method: "m2", Values: url.Values{"n2": {"v2"}}},
 			},
@@ -43,7 +43,7 @@ func Test_ParseForms(t *testing.T) {
 			   <input type="radio" name="n1" value="v2">
 			   <input type="radio" name="n1" value="v3">
 			</form></html>`,
-			[]htmlForm{{Values: url.Values{}}},
+			[]*htmlForm{{Values: url.Values{}}},
 		},
 		{
 			"form with radio buttons",
@@ -52,7 +52,7 @@ func Test_ParseForms(t *testing.T) {
 			   <input type="radio" name="n1" value="v2">
 			   <input type="radio" name="n1" value="v3" checked>
 			</form></html>`,
-			[]htmlForm{{Values: url.Values{"n1": {"v3"}}}},
+			[]*htmlForm{{Values: url.Values{"n1": {"v3"}}}},
 		},
 		{
 			"form with checkboxes",
@@ -61,12 +61,12 @@ func Test_ParseForms(t *testing.T) {
 			   <input type="checkbox" name="n2" value="v2">
 			   <input type="checkbox" name="n3" value="v3" checked>
 			</form></html>`,
-			[]htmlForm{{Values: url.Values{"n1": {"v1"}, "n3": {"v3"}}}},
+			[]*htmlForm{{Values: url.Values{"n1": {"v1"}, "n3": {"v3"}}}},
 		},
 		{
 			"single form with textarea",
 			`<html><form><textarea name="n1">v1</textarea></form></html>`,
-			[]htmlForm{{Values: url.Values{"n1": {"v1"}}}},
+			[]*htmlForm{{Values: url.Values{"n1": {"v1"}}}},
 		},
 	}
 

diff --git a/script/lint.sh b/script/lint.sh
@@ -19,9 +19,10 @@ fail() {
   EXIT_CODE=1
 }
 
-# install golangci-lint bin/golangci-lint doesn't exist with the correct version
-if ! "$BIN"/golangci-lint --version 2> /dev/null | grep -q "$GOLANGCI_LINT_VERSION"; then
+# install golangci-lint and custom-gcl in ./bin if they don't exist with the correct version
+if ! "$BIN"/custom-gcl --version 2> /dev/null | grep -q "$GOLANGCI_LINT_VERSION"; then
   GOBIN="$BIN" go install "github.com/golangci/golangci-lint/cmd/golangci-lint@v$GOLANGCI_LINT_VERSION"
+  "$BIN"/golangci-lint custom && mv ./custom-gcl "$BIN"
 fi
 
 MOD_DIRS="$(git ls-files '*go.mod' | xargs dirname | sort)"
@@ -33,9 +34,9 @@ for dir in $MOD_DIRS; do
     cd "$dir"
     # github actions output when running in an action
     if [ -n "$GITHUB_ACTIONS" ]; then
-      "$BIN"/golangci-lint run --path-prefix "$dir" --out-format colored-line-number
+      "$BIN"/custom-gcl run --path-prefix "$dir" --out-format colored-line-number
     else
-      "$BIN"/golangci-lint run --path-prefix "$dir"
+      "$BIN"/custom-gcl run --path-prefix "$dir"
     fi
   ) || fail "failed linting $dir"
 done

diff --git a/tools/sliceofpointers/go.mod b/tools/sliceofpointers/go.mod
@@ -0,0 +1,13 @@
+module tools/sliceofpointers
+
+go 1.22.0
+
+require (
+	github.com/golangci/plugin-module-register v0.1.1
+	golang.org/x/tools v0.29.0
+)
+
+require (
+	golang.org/x/mod v0.22.0 // indirect
+	golang.org/x/sync v0.10.0 // indirect
+)
diff --git a/tools/sliceofpointers/go.sum b/tools/sliceofpointers/go.sum
@@ -0,0 +1,10 @@
+github.com/golangci/plugin-module-register v0.1.1 h1:TCmesur25LnyJkpsVrupv1Cdzo+2f7zX0H6Jkw1Ol6c=
+github.com/golangci/plugin-module-register v0.1.1/go.mod h1:TTpqoB6KkwOJMV8u7+NyXMrkwwESJLOkfl9TxR1DGFc=
+github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI=
+github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY=
+golang.org/x/mod v0.22.0 h1:D4nJWe9zXqHOmWqj4VMOJhvzj7bEZg4wEYa759z1pH4=
+golang.org/x/mod v0.22.0/go.mod h1:6SkKJ3Xj0I0BrPOZoBy3bdMptDDU9oJrpohJ3eWZ1fY=
+golang.org/x/sync v0.10.0 h1:3NQrjDixjgGwUOCaF8w2+VYHv0Ve/vGYSbdkTa98gmQ=
+golang.org/x/sync v0.10.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk=
+golang.org/x/tools v0.29.0 h1:Xx0h3TtM9rzQpQuR4dKLrdglAmCEN5Oi+P74JdhdzXE=
+golang.org/x/tools v0.29.0/go.mod h1:KMQVMRsVxU6nHCFXrBPhDB8XncLNLM0lIy/F14RP588=