docs: Update CONTRIBUTING.md (#4341)

Author: alexandear

CONTRIBUTING.md

@@ -180,7 +180,6 @@ And the returned type `Repository` will have comments like this:
 // Repository represents a GitHub repository.
 type Repository struct {
 	ID     *int64  `json:"id,omitempty"`
-	NodeID *string `json:"node_id,omitempty"`
 	Owner  *User   `json:"owner,omitempty"`
 	// ...
 }
@@ -222,6 +221,28 @@ For example, methods defined at <https://docs.github.com/en/rest/webhooks/repos>
 
 [GitHub API documentation]: https://docs.github.com/en/rest
 
+### Services
+
+The API is split into services, one per logical area of the GitHub API
+(e.g. `RepositoriesService`, `UsersService`). Each service is a named type
+over the shared `service` struct, which holds a back-reference to the `Client`:
+
+```go
+type service struct {
+	client *Client
+}
+
+type RepositoriesService service
+```
+
+All services share a single `service` value embedded in the `Client` as
+`common`, so adding a service does not allocate. To add a new service:
+
+1. Add a field for it on the `Client` struct, keeping the list alphabetical
+2. Wire it up in `newClient` (alongside the other `c.X = (*XService)(&c.common)` lines, also kept alphabetical).
+3. Declare the service type in the service's file (e.g. `repos.go`) and hang the
+   methods off it, using `s` as the receiver (see [Receiver Names](#receiver-names)).
+
 ### Naming Conventions
 
 #### Receiver Names
@@ -263,6 +284,7 @@ Common method name prefixes:
 - `result` - Result from API call
 - `err` - Error
 - `opts` - Options parameter
+- `body` - Body parameter
 - `owner` - Repository owner (username or organization)
 - `repo` - Repository name
 - `org` - Organization name
@@ -283,30 +305,45 @@ See [#3644][] and [#3887][] for background discussion.
 [#3644]: https://github.com/google/go-github/pull/3644
 [#3887]: https://github.com/google/go-github/pull/3887
 
+#### Creating Pointers
+
+Pointer fields are common because many request and response fields are
+optional. Use the generic `Ptr` helper to take the address of a literal
+instead of declaring an intermediate variable:
+
+```go
+repo := &Repository{
+	Name:    Ptr("go-github"),
+	Private: Ptr(true),
+	ID:      Ptr(int64(1)),
+}
+```
+
 #### Request Option Types
 
-Request option types (for query parameters) are named after the method they
+Request option types should be used for query parameters and named after the method they
 modify, with the suffix `Options`:
 
 ```go
 type RepositoryListOptions struct {
-	Type string `url:"type,omitempty"`
-	Sort string `url:"sort,omitempty"`
+	Type      string `url:"type,omitempty"`
+	Sort      string `url:"sort,omitempty"`
 	Direction string `url:"direction,omitempty"`
 	ListOptions
 }
 ```
 
 #### Request Body Types
 
-Request body types (for POST/PUT/PATCH requests) should use the `Request`
+Request body types for POST/PUT/PATCH requests should use the `Request`
 suffix for create and update operations:
 
 ```go
-type CreateSelfHostedRunnerGroupRequest struct {
-	Name         string `json:"name"`
-	RunnerGroupId int64 `json:"runner_group_id"`
-	Visibility   string `json:"visibility,omitempty"`
+type CreateHostedRunnerRequest struct {
+	Name           string `json:"name"`
+	RunnerGroupID  int64  `json:"runner_group_id"`
+	MaximumRunners *int64 `json:"maximum_runners,omitempty"`
+	// ...
 }
 ```
 
@@ -317,9 +354,9 @@ any suffix:
 
 ```go
 type Repository struct {
-	ID *int64 `json:"id,omitempty"`
-	Name *string `json:"name,omitempty"`
-	FullName *string `json:"full_name,omitempty"`
+	ID          *int64  `json:"id,omitempty"`
+	Name        *string `json:"name,omitempty"`
+	FullName    *string `json:"full_name,omitempty"`
 	Description *string `json:"description,omitempty"`
 	// ...
 }
@@ -336,31 +373,34 @@ type Repository struct {
 
 #### Request Bodies
 
-Required fields should be non-pointer types without `omitempty`:
+Required fields should be non-pointer types without `omitempty`.
+Optional fields should be pointer types with `omitempty`.
+Use `omitzero` for structs and `time.Time` where you want to omit
+empty values (not just nil). For slices and maps, `omitzero` has the
+opposite behavior: it keeps empty (non-nil) values and only omits nil
+values.
 
 ```go
-type SecretScanningAlertUpdateOptions struct {
-	State string `json:"state"`
-	// ...
-}
-```
+type RepositoryRuleset struct {
+	// ID is optional.
+	ID *int64 `json:"id,omitempty"`
 
-Optional fields should be pointer types with `omitempty`:
+	// Name is required.
+	Name string `json:"name"`
 
-```go
-type SecretScanningAlertUpdateOptions struct {
-	State string `json:"state"`
-	Resolution *string `json:"resolution,omitempty"`
-	ResolutionComment *string `json:"resolution_comment,omitempty"`
+	// Target is optional struct.
+	Target *RulesetTarget `json:"target,omitempty"`
+
+	// BypassActors is optional.
+	BypassActors []*BypassActor `json:"bypass_actors,omitzero"`
+
+	// CreatedAt is optional.
+	CreatedAt *Timestamp `json:"created_at,omitempty"`
+
+	// ...
 }
 ```
 
-Use `omitzero` for structs and `time.Time` where you want to omit
-empty values (not just nil). For slices and maps, `omitzero` has the
-opposite behavior: it keeps empty (non-nil) values and only omits nil
-values. See `RepositoryRuleset` for examples of `omitzero` usage with
-both slices and structs.
-
 For optional boolean fields where you need to distinguish between `false`
 and "not set", use `*bool` with `omitzero`.
 
@@ -369,22 +409,17 @@ and "not set", use `*bool` with `omitzero`.
 Follow the same conventions as request bodies for `omitempty` and
 `omitzero` usage. Optional fields should use pointer types with
 `omitempty`, and required fields should prefer non-pointer types.
-See [Common Types](#common-types) for conventions on ID, Node ID,
-Timestamp, and Boolean fields.
+See [Common Types](#common-types) for conventions on ID, Node ID, and Timestamp.
 
 ### URL Tags for Query Parameters
 
 All fields should use `url` tags with `omitempty` to omit zero values
 from the query string:
 
 ```go
-type RepositoryContentGetOptions struct {
-	Ref string `url:"ref,omitempty"`
-}
-
 type RepositoryListOptions struct {
-	Type string `url:"type,omitempty"`
-	Sort string `url:"sort,omitempty"`
+	Type      string `url:"type,omitempty"`
+	Sort      string `url:"sort,omitempty"`
 	Direction string `url:"direction,omitempty"`
 	ListOptions
 }
@@ -400,7 +435,7 @@ Use `ListOptions` for APIs that use page/per_page parameters:
 
 ```go
 type ListOptions struct {
-	Page int `url:"page,omitempty"`
+	Page    int `url:"page,omitempty"`
 	PerPage int `url:"per_page,omitempty"`
 }
 ```
@@ -411,13 +446,13 @@ Use `ListCursorOptions` for APIs that use cursor-based pagination:
 
 ```go
 type ListCursorOptions struct {
-	Page string `url:"page,omitempty"`
-	PerPage int `url:"per_page,omitempty"`
-	First int `url:"first,omitempty"`
-	Last int `url:"last,omitempty"`
-	After string `url:"after,omitempty"`
-	Before string `url:"before,omitempty"`
-	Cursor string `url:"cursor,omitempty"`
+	Page    string `url:"page,omitempty"`
+	PerPage int    `url:"per_page,omitempty"`
+	First   int    `url:"first,omitempty"`
+	Last    int    `url:"last,omitempty"`
+	After   string `url:"after,omitempty"`
+	Before  string `url:"before,omitempty"`
+	Cursor  string `url:"cursor,omitempty"`
 }
 ```
 
@@ -436,32 +471,23 @@ in `gen-iterators.go` can be adjusted — including `useCursorPagination`,
 
 #### ID Fields
 
-GitHub API IDs are always `int64`. In request bodies, use non-pointer `int64`
-if the ID is required and `*int64` if the ID is optional. In response bodies,
-use non-pointer `int64` if the ID is required and `*int64` with `omitempty`
-if the ID is optional:
+GitHub API IDs are usually `int64`. Use non-pointer `int64`
+if the ID is required and `*int64` if the ID is optional.
 
 ```go
-// Request body — required ID
 type CreateHostedRunnerRequest struct {
 	RunnerGroupID int64 `json:"runner_group_id"`
-}
-
-// Response body — optional ID
-type Repository struct {
-	ID *int64 `json:"id,omitempty"`
 	// ...
 }
 ```
 
 #### Node ID Fields
 
-Node IDs are always `string`. In response bodies, `NodeID` is typically
-required and should use a non-pointer type:
+Node IDs are usually `string`:
 
 ```go
-type Repository struct {
-	NodeID *string `json:"node_id,omitempty"`
+type IssueFieldValue struct {
+	NodeID string `json:"node_id"`
 	// ...
 }
 ```
@@ -473,12 +499,76 @@ Use the `Timestamp` type for all date/time fields:
 ```go
 type Repository struct {
 	CreatedAt *Timestamp `json:"created_at,omitempty"`
-	UpdatedAt *Timestamp `json:"updated_at,omitempty"`
-	PushedAt *Timestamp `json:"pushed_at,omitempty"`
 	// ...
 }
 ```
 
+### Generated Code
+
+Some files are generated and must never be edited by hand.
+When you add or change a struct, run `script/generate.sh` to regenerate them.
+
+So after adding a field you typically only write the struct field itself;
+the accessor and stringify code follow from `script/generate.sh`.
+`script/lint.sh` will fail if these files are out of date.
+Documentation links and `//meta:operation` directives are updated
+by the same script (see [Code Comments](#code-comments)).
+
+### Testing
+
+Tests use a real `httptest` server rather than mocks. Call `setup` to get a
+`Client` pointed at a test server plus the `mux` to register handlers on, then
+assert on the request and the decoded response. A typical test looks like this:
+
+```go
+func TestRepositoriesService_GetByID(t *testing.T) {
+	t.Parallel()
+	client, mux, _ := setup(t)
+
+	mux.HandleFunc("/repositories/1", func(w http.ResponseWriter, r *http.Request) {
+		testMethod(t, r, "GET")
+		fmt.Fprint(w, `{"id":1,"name":"n"}`)
+	})
+
+	ctx := t.Context()
+	got, _, err := client.Repositories.GetByID(ctx, 1)
+	if err != nil {
+		t.Fatalf("Repositories.GetByID returned error: %v", err)
+	}
+
+	want := &Repository{ID: Ptr(int64(1)), Name: Ptr("n")}
+	if !cmp.Equal(got, want) {
+		t.Errorf("Repositories.GetByID returned %+v, want %+v", got, want)
+	}
+
+	const methodName = "GetByID"
+	testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
+		got, resp, err := client.Repositories.GetByID(ctx, 1)
+		if got != nil {
+			t.Errorf("testNewRequestAndDoFailure %v = %#v, want nil", methodName, got)
+		}
+		return resp, err
+	})
+}
+```
+
+Conventions to follow:
+
+- Call `t.Parallel()` and use `t.Context()` for the request context.
+- Register handlers on `mux` with relative paths (a leading `/`); the test
+  server fails requests built from absolute URLs to catch that mistake.
+- Use the shared helpers instead of reimplementing assertions:
+  - `testMethod` - asserts the HTTP method.
+  - `testFormValues` - asserts query parameters.
+  - `testHeader` - asserts a request header.
+  - `testJSONBody` / `testPlainBody` - assert the request body.
+  - `testNewRequestAndDoFailure` - exercises the request-building and
+    request-doing error paths for a method.
+  - `testBadOptions` - asserts that invalid options return an error.
+- Use `testJSONMarshal` to verify a type round-trips to and from the expected JSON.
+- Compare values with `cmp.Equal` and construct optional fields with `Ptr`
+  (see [Creating Pointers](#creating-pointers)).
+
 ## Metadata
 
 GitHub publishes [OpenAPI descriptions of their API][]. We use these