Skip to content

docs: Update CONTRIBUTING.md #4341

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

Open
alexandear wants to merge 1 commit into
base: master
Choose a base branch
from
+152 −62
Open

docs: Update CONTRIBUTING.md #4341

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
214 changes: 152 additions & 62 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
// ...
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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"`
// ...
}
```

Expand All @@ -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"`
// ...
}
Expand All @@ -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`.

Expand All @@ -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
}
Expand All @@ -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"`
}
```
Expand All @@ -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"`
}
```

Expand All @@ -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"`
// ...
}
```
Expand All @@ -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
Expand Down
Loading