-
Notifications
You must be signed in to change notification settings - Fork 2.2k
docs: extend CONTRIBUTING.md with code guidelines #4202
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -199,6 +199,355 @@ description. | |||||||||
|
|
||||||||||
| [Go Doc Comments]: https://go.dev/doc/comment | ||||||||||
|
|
||||||||||
| ## Code Guidelines | ||||||||||
|
|
||||||||||
| This section documents common code patterns and conventions used throughout | ||||||||||
| the go-github repository. Following these guidelines helps maintain consistency | ||||||||||
| and makes the codebase easier to understand and maintain. | ||||||||||
|
|
||||||||||
| ### Naming Conventions | ||||||||||
|
|
||||||||||
| #### File Names | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we remove the "Other notes on code organization" section? |
||||||||||
|
|
||||||||||
| Files are organized by service and API endpoint, following the pattern | ||||||||||
| `{service}_{api}.go`. For example: | ||||||||||
| - `repos_contents.go` - Repository contents API methods | ||||||||||
| - `users_ssh_signing_keys.go` - User SSH signing keys API methods | ||||||||||
| - `orgs_rules.go` - Organization rules API methods | ||||||||||
|
|
||||||||||
| Test files follow the pattern `{service}_{api}_test.go`. | ||||||||||
|
|
||||||||||
| #### Receiver Names | ||||||||||
|
|
||||||||||
| Service method receivers consistently use the single letter `s`: | ||||||||||
|
|
||||||||||
| ```go | ||||||||||
| func (s *RepositoriesService) Get(ctx context.Context, owner, repo string) (*Repository, *Response, error) { | ||||||||||
| // ... | ||||||||||
| } | ||||||||||
| ``` | ||||||||||
|
|
||||||||||
| #### Request Option Types | ||||||||||
|
|
||||||||||
| Request option types (for query parameters) are named after the method they | ||||||||||
| modify, with the suffix `Options`: | ||||||||||
|
|
||||||||||
| ```go | ||||||||||
| type RepositoryListOptions struct { | ||||||||||
| 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) are named after the method | ||||||||||
| they modify, with the suffix `Options`: | ||||||||||
|
|
||||||||||
| ```go | ||||||||||
| type RepositoryContentFileOptions struct { | ||||||||||
| Message *string `json:"message,omitempty"` | ||||||||||
| Content []byte `json:"content"` | ||||||||||
| SHA *string `json:"sha,omitempty"` | ||||||||||
| Branch *string `json:"branch,omitempty"` | ||||||||||
| Author *CommitAuthor `json:"author,omitempty"` | ||||||||||
| Committer *CommitAuthor `json:"committer,omitempty"` | ||||||||||
| } | ||||||||||
| ``` | ||||||||||
|
|
||||||||||
| #### Response Types | ||||||||||
|
|
||||||||||
| Response types are named after the resource they represent, typically without | ||||||||||
| any suffix: | ||||||||||
|
|
||||||||||
| ```go | ||||||||||
| type Repository struct { | ||||||||||
| ID *int64 `json:"id,omitempty"` | ||||||||||
| Name *string `json:"name,omitempty"` | ||||||||||
| FullName *string `json:"full_name,omitempty"` | ||||||||||
| Description *string `json:"description,omitempty"` | ||||||||||
| // ... | ||||||||||
| } | ||||||||||
| ``` | ||||||||||
|
|
||||||||||
| #### Method and Variable Naming | ||||||||||
|
|
||||||||||
| Methods use descriptive names that clearly indicate their action: | ||||||||||
| - `Get` - Retrieve a single resource | ||||||||||
| - `List` - Retrieve multiple resources (supports pagination) | ||||||||||
| - `Create` - Create a new resource | ||||||||||
| - `Update` - Update an existing resource | ||||||||||
| - `Delete` - Delete a resource | ||||||||||
| - `Edit` - Edit an existing resource (alternative to Update) | ||||||||||
|
|
||||||||||
| Common local variable names: | ||||||||||
| - `ctx` - Context | ||||||||||
| - `u` - URL string | ||||||||||
| - `req` - HTTP request | ||||||||||
| - `resp` - HTTP response | ||||||||||
| - `result` - Result from API call | ||||||||||
| - `err` - Error | ||||||||||
|
|
||||||||||
| #### Common Variable Names | ||||||||||
|
|
||||||||||
| These variable names are used consistently throughout the codebase: | ||||||||||
| - `owner` - Repository owner (username or organization) | ||||||||||
| - `repo` - Repository name | ||||||||||
| - `org` - Organization name | ||||||||||
| - `user` - Username | ||||||||||
| - `team` - Team name or slug | ||||||||||
| - `project` - Project name or number | ||||||||||
|
|
||||||||||
| #### Enterprise and Organization Scoped Methods | ||||||||||
|
|
||||||||||
| Methods that operate on enterprise or organization resources include the scope | ||||||||||
| in their name: | ||||||||||
|
|
||||||||||
| ```go | ||||||||||
| // Enterprise-scoped methods | ||||||||||
| func (s *EnterpriseService) GetLicenseInfo(ctx context.Context) (*LicenseInfo, *Response, error) | ||||||||||
|
|
||||||||||
| // Organization-scoped methods | ||||||||||
| func (s *OrganizationsService) ListMembers(ctx context.Context, org string, opts *OrganizationListMembersOptions) ([]*User, *Response, error) | ||||||||||
| ``` | ||||||||||
|
Comment on lines
+303
to
+314
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, I think this subsection may be incorrect - see #3761. |
||||||||||
|
|
||||||||||
| #### Common Structs | ||||||||||
|
|
||||||||||
| These common structs are used throughout the codebase: | ||||||||||
| - `ListOptions` - For offset-based pagination (page/per_page) | ||||||||||
| - `ListCursorOptions` - For cursor-based pagination | ||||||||||
| - `UploadOptions` - For file uploads | ||||||||||
| - `Response` - Wraps the HTTP response | ||||||||||
|
|
||||||||||
| ### JSON Tags for Request Bodies | ||||||||||
|
|
||||||||||
| When defining structs that represent a request body (sent via POST/PUT/PATCH): | ||||||||||
|
|
||||||||||
| 1. **Required fields** should be non-pointer types without `omitempty`: | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please do not use excessive LLM-like formatting:
Suggested change
|
||||||||||
|
|
||||||||||
| ```go | ||||||||||
| type SecretScanningAlertUpdateOptions struct { | ||||||||||
| State string `json:"state"` // Required field | ||||||||||
| // ... | ||||||||||
|
Comment on lines
+332
to
+333
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please use TABs instead of spaces for all examples:
Suggested change
|
||||||||||
| } | ||||||||||
| ``` | ||||||||||
|
|
||||||||||
| 2. **Optional fields** should be pointer types with `omitempty`: | ||||||||||
|
|
||||||||||
| ```go | ||||||||||
| type SecretScanningAlertUpdateOptions struct { | ||||||||||
| State string `json:"state"` | ||||||||||
| Resolution *string `json:"resolution,omitempty"` | ||||||||||
| ResolutionComment *string `json:"resolution_comment,omitempty"` | ||||||||||
| } | ||||||||||
| ``` | ||||||||||
|
|
||||||||||
| 3. **Use `omitzero` for structs and slices** where you want to omit empty values | ||||||||||
| (not just nil): | ||||||||||
|
|
||||||||||
| ```go | ||||||||||
| type ActivityNotificationOptions struct { | ||||||||||
| SubjectType string `json:"subject_type,omitempty"` | ||||||||||
| Subject *string `json:"subject,omitempty"` | ||||||||||
| LastReadAt Timestamp `json:"last_read_at,omitzero"` | ||||||||||
| } | ||||||||||
| ``` | ||||||||||
|
|
||||||||||
| ### URL Tags for Query Parameters | ||||||||||
|
|
||||||||||
| When defining structs that represent query parameters (sent via URL): | ||||||||||
|
|
||||||||||
| 1. **All fields should be non-pointer types** with `url` tags: | ||||||||||
|
|
||||||||||
| ```go | ||||||||||
| type RepositoryContentGetOptions struct { | ||||||||||
| Ref string `url:"ref,omitempty"` | ||||||||||
| } | ||||||||||
|
|
||||||||||
| type ListOptions struct { | ||||||||||
| Page int `url:"page,omitempty"` | ||||||||||
| PerPage int `url:"per_page,omitempty"` | ||||||||||
| } | ||||||||||
| ``` | ||||||||||
|
|
||||||||||
| 2. **Use `omitempty` to omit zero values** from the query string: | ||||||||||
|
|
||||||||||
| ```go | ||||||||||
| type RepositoryListOptions struct { | ||||||||||
| Type string `url:"type,omitempty"` | ||||||||||
| Sort string `url:"sort,omitempty"` | ||||||||||
| Direction string `url:"direction,omitempty"` | ||||||||||
| ListOptions | ||||||||||
| } | ||||||||||
| ``` | ||||||||||
|
|
||||||||||
| ### Pagination | ||||||||||
|
|
||||||||||
| The go-github library supports two types of pagination: | ||||||||||
|
|
||||||||||
| #### Offset-based Pagination | ||||||||||
|
|
||||||||||
| Use `ListOptions` for APIs that use page/per_page parameters: | ||||||||||
|
|
||||||||||
| ```go | ||||||||||
| type ListOptions struct { | ||||||||||
| // For paginated result sets, page of results to retrieve. | ||||||||||
| Page int `url:"page,omitempty"` | ||||||||||
|
|
||||||||||
| // For paginated result sets, the number of results to include per page. | ||||||||||
| PerPage int `url:"per_page,omitempty"` | ||||||||||
| } | ||||||||||
| ``` | ||||||||||
|
|
||||||||||
| #### Cursor-based Pagination | ||||||||||
|
|
||||||||||
| Use `ListCursorOptions` for APIs that use cursor-based pagination: | ||||||||||
|
|
||||||||||
| ```go | ||||||||||
| type ListCursorOptions struct { | ||||||||||
| // For paginated result sets, page of results to retrieve. | ||||||||||
| Page string `url:"page,omitempty"` | ||||||||||
|
|
||||||||||
| // For paginated result sets, the number of results to include per page. | ||||||||||
| PerPage int `url:"per_page,omitempty"` | ||||||||||
|
|
||||||||||
| // For paginated result sets, the number of results per page (max 100), starting from the first matching result. | ||||||||||
| // This parameter must not be used in combination with last. | ||||||||||
| First int `url:"first,omitempty"` | ||||||||||
|
|
||||||||||
| // For paginated result sets, the number of results per page (max 100), starting from the last matching result. | ||||||||||
| // This parameter must not be used in combination with first. | ||||||||||
| Last int `url:"last,omitempty"` | ||||||||||
|
|
||||||||||
| // A cursor, as given in the Link header. If specified, the query only searches for events after this cursor. | ||||||||||
| After string `url:"after,omitempty"` | ||||||||||
|
|
||||||||||
| // A cursor, as given in the Link header. If specified, the query only searches for events before this cursor. | ||||||||||
| Before string `url:"before,omitempty"` | ||||||||||
|
|
||||||||||
| // A cursor, as given in the Link header. If specified, the query continues the search using this cursor. | ||||||||||
| Cursor string `url:"cursor,omitempty"` | ||||||||||
| } | ||||||||||
| ``` | ||||||||||
|
|
||||||||||
| #### Pagination Best Practices | ||||||||||
|
|
||||||||||
| 1. **Embed pagination options** in your option structs: | ||||||||||
|
|
||||||||||
| ```go | ||||||||||
| type RepositoryListOptions struct { | ||||||||||
| Type string `url:"type,omitempty"` | ||||||||||
| Sort string `url:"sort,omitempty"` | ||||||||||
| Direction string `url:"direction,omitempty"` | ||||||||||
| ListOptions // Embed for pagination | ||||||||||
| } | ||||||||||
| ``` | ||||||||||
|
|
||||||||||
| 2. **Handle nil options** in your methods: | ||||||||||
|
|
||||||||||
| ```go | ||||||||||
| func (s *RepositoriesService) List(ctx context.Context, user string, opts *RepositoryListOptions) ([]*Repository, *Response, error) { | ||||||||||
| if opts == nil { | ||||||||||
| opts = &RepositoryListOptions{} | ||||||||||
| } | ||||||||||
| // ... | ||||||||||
| } | ||||||||||
| ``` | ||||||||||
|
Comment on lines
+448
to
+457
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can be removed. We have the |
||||||||||
|
|
||||||||||
| 3. **Use iterators** for list methods that support pagination. The library | ||||||||||
| automatically generates iterator methods (e.g., `ListIter`) for methods | ||||||||||
| that start with `List` and return a slice. | ||||||||||
|
|
||||||||||
| ### Common Types | ||||||||||
|
|
||||||||||
| These types are used consistently throughout the codebase: | ||||||||||
|
|
||||||||||
| #### ID Fields | ||||||||||
|
|
||||||||||
| GitHub API IDs are always `int64`: | ||||||||||
|
|
||||||||||
| ```go | ||||||||||
| type Repository struct { | ||||||||||
| ID *int64 `json:"id,omitempty"` | ||||||||||
| // ... | ||||||||||
| } | ||||||||||
|
|
||||||||||
| type User struct { | ||||||||||
| ID *int64 `json:"id,omitempty"` | ||||||||||
| // ... | ||||||||||
| } | ||||||||||
| ``` | ||||||||||
|
|
||||||||||
| #### Node ID Fields | ||||||||||
|
|
||||||||||
| Node IDs are always `string`: | ||||||||||
|
|
||||||||||
| ```go | ||||||||||
| type Repository struct { | ||||||||||
| ID *int64 `json:"id,omitempty"` | ||||||||||
| NodeID *string `json:"node_id,omitempty"` | ||||||||||
| // ... | ||||||||||
| } | ||||||||||
| ``` | ||||||||||
|
|
||||||||||
| #### Timestamp Fields | ||||||||||
|
|
||||||||||
| 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"` | ||||||||||
| // ... | ||||||||||
| } | ||||||||||
| ``` | ||||||||||
|
|
||||||||||
| #### Boolean Fields | ||||||||||
|
|
||||||||||
| Boolean fields are always `*bool` (pointer to bool) to distinguish between | ||||||||||
| `false` and `not set`: | ||||||||||
|
|
||||||||||
| ```go | ||||||||||
| type Repository struct { | ||||||||||
| Private *bool `json:"private,omitempty"` | ||||||||||
| Fork *bool `json:"fork,omitempty"` | ||||||||||
| // ... | ||||||||||
| } | ||||||||||
| ``` | ||||||||||
|
|
||||||||||
| ### Generation Patterns | ||||||||||
|
|
||||||||||
| The go-github repository uses code generation for several purposes: | ||||||||||
|
|
||||||||||
| #### Generated Accessors | ||||||||||
|
|
||||||||||
| The `github-accessors.go` file contains generated getter and setter methods | ||||||||||
| for struct fields. These are generated automatically and should not be edited | ||||||||||
| manually. | ||||||||||
|
|
||||||||||
| #### Generated Iterators | ||||||||||
|
|
||||||||||
| Iterator methods are automatically generated for list methods that: | ||||||||||
| 1. Start with `List` | ||||||||||
| 2. Return a slice | ||||||||||
| 3. Accept pagination options | ||||||||||
|
|
||||||||||
| For example, `RepositoriesService.List` automatically gets a `ListIter` method. | ||||||||||
|
|
||||||||||
| #### Generated Documentation | ||||||||||
|
|
||||||||||
| Documentation links are automatically generated from `openapi_operations.yaml`. | ||||||||||
| Run `script/generate.sh` to update these links. | ||||||||||
|
Comment on lines
+540
to
+543
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These lines are not needed. They are already in "Submitting a patch", "Metadata", and "Scripts". Can be removed. |
||||||||||
|
|
||||||||||
| #### Linter Rules | ||||||||||
|
|
||||||||||
| The repository uses custom linter rules to enforce consistency: | ||||||||||
| - `sliceofpointers` - Ensures slice fields use pointers | ||||||||||
| - `structfield` - Ensures struct fields follow naming conventions | ||||||||||
|
Comment on lines
+545
to
+549
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have more linters. Let's simply remove this section. |
||||||||||
|
|
||||||||||
| ## Metadata | ||||||||||
|
|
||||||||||
| GitHub publishes [OpenAPI descriptions of their API][]. We use these | ||||||||||
|
|
||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the "Code Comments" section above should be under "Code Guidelines".