diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1c51f03f6a1..d8f10c74212 100644 --- a/CONTRIBUTING.md +++ b/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 [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,15 +305,29 @@ 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 } @@ -299,14 +335,15 @@ type RepositoryListOptions struct { #### 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,8 +409,7 @@ 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 @@ -378,13 +417,9 @@ 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