docs: extend CONTRIBUTING.md with code guidelines by jlaportebot · Pull Request #4202 · google/go-github

jlaportebot · 2026-05-08T22:10:06Z

Summary

This PR extends CONTRIBUTING.md with a comprehensive section on code guidelines and conventions, addressing issue #4023.

Changes

Added a new "Code Guidelines" section that documents:

Naming Conventions
- File names ({service}_{api}.go pattern)
- Receiver names (consistently use 's')
- Request option types
- Request body types
- Response types
- Method and variable naming
- Common variable names (owner, repo, org, user, team, project)
- Enterprise and organization scoped methods
- Common structs (ListOptions, ListCursorOptions, UploadOptions, Response)
JSON Tags for Request Bodies
- Required fields: non-pointer types without omitempty
- Optional fields: pointer types with omitempty
- Use omitzero for structs and slices
URL Tags for Query Parameters
- All fields should be non-pointer types with url tags
- Use omitempty to omit zero values
Pagination
- Offset-based pagination (ListOptions)
- Cursor-based pagination (ListCursorOptions)
- Pagination best practices (embedding options, handling nil, using iterators)
Common Types
- ID fields: always int64
- Node ID fields: always string
- Timestamp fields: use Timestamp type
- Boolean fields: always *bool
Generation Patterns
- Generated accessors
- Generated iterators
- Generated documentation
- Linter rules (sliceofpointers, structfield)

Benefits

Helps new contributors understand the codebase better
Reduces review burden by making expectations explicit
Improves code consistency across the project
Documents patterns that are frequently discussed in reviews

Testing

This is a documentation-only change. No code changes or tests needed.

Closes #4023

Add comprehensive section documenting common code patterns and conventions: - Naming conventions (files, receivers, types, methods, variables) - JSON tags for request bodies (required vs optional fields, omitzero) - URL tags for query parameters (non-pointer fields with omitempty) - Pagination patterns (ListOptions, ListCursorOptions, best practices) - Common types (ID, NodeID, Timestamp, boolean fields) - Generation patterns (accessors, iterators, documentation, linter rules) This addresses issue google#4023 and helps new contributors understand the codebase better, reducing review burden and improving code consistency.

gmlewis

Thank you, @jlaportebot.
LGTM.

cc: @alexandear

codecov · 2026-05-08T22:22:26Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.34%. Comparing base (6c58592) to head (dfe7486).
⚠️ Report is 13 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4202      +/-   ##
==========================================
- Coverage   93.71%   93.34%   -0.37%     
==========================================
  Files         209      209              
  Lines       19772    19998     +226     
==========================================
+ Hits        18529    18667     +138     
- Misses       1046     1114      +68     
- Partials      197      217      +20

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

alexandear

@jlaportebot Thank you. I left a few comments regarding excessive formatting and redundant information. Could you also review the entire CONTRIBUTING.md document and consolidate duplicated content?

Also, I think this PR should be reviewed by everyone listed in the REVIEWERS file.

alexandear · 2026-05-10T18:39:08Z


 [Go Doc Comments]: https://go.dev/doc/comment

+## Code Guidelines


I think the "Code Comments" section above should be under "Code Guidelines".

alexandear · 2026-05-10T18:41:29Z

+#### Generated Documentation
+
+Documentation links are automatically generated from `openapi_operations.yaml`.
+Run `script/generate.sh` to update these links.


These lines are not needed.

They are already in "Submitting a patch", "Metadata", and "Scripts".

Can be removed.

alexandear · 2026-05-10T18:42:12Z

+#### Linter Rules
+
+The repository uses custom linter rules to enforce consistency:
+- `sliceofpointers` - Ensures slice fields use pointers
+- `structfield` - Ensures struct fields follow naming conventions


We have more linters. Let's simply remove this section.

alexandear · 2026-05-10T18:43:30Z

+    State string `json:"state"`  // Required field
+    // ...


Please use TABs instead of spaces for all examples:

Suggested change

State string `json:"state"` // Required field

// ...

State string `json:"state"` // Required field

// ...

alexandear · 2026-05-10T18:46:24Z

+
+### Naming Conventions
+
+#### File Names


Should we remove the "Other notes on code organization" section?

alexandear · 2026-05-10T18:50:55Z

+#### 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)
+```


Actually, I think this subsection may be incorrect - see #3761.

alexandear · 2026-05-10T18:51:41Z

+
+When defining structs that represent a request body (sent via POST/PUT/PATCH):
+
+1. **Required fields** should be non-pointer types without `omitempty`:


Please do not use excessive LLM-like formatting:

Suggested change

1. **Required fields** should be non-pointer types without `omitempty`:

1. Required fields should be non-pointer types without `omitempty`:

alexandear · 2026-05-10T18:54:04Z

+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{}
+    }
+    // ...
+}
+```


This can be removed. We have the addOptions to handle nil options.

alexandear · 2026-05-12T13:32:26Z

@stevehipwell @zyfy29 @Not-Dhananjay-Mishra @munlicode please review when you have time

stevehipwell · 2026-05-12T14:23:50Z

@alexandear did you not want your review comments addressing before someone else weighs in?

munlicode · 2026-05-13T06:31:55Z

+
+### Naming Conventions
+
+#### File Names


Maybe remove "Other notes on code organization" as @alexandear suggested, and put this:

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`. These services map directly to how the [GitHub API documentation](https://docs.github.com/en/rest) is organized, so use that as your guide for where to put new methods. For example, methods defined at <https://docs.github.com/en/rest/webhooks/repos> live in [repos_hooks.go][https://github.com/google/go-github/blob/master/github/repos_hooks.go].

alexandear · 2026-05-13T08:25:31Z

@alexandear did you not want your review comments addressing before someone else weighs in?

@stevehipwell I prefer that all reviewers contribute to the guidelines regardless of my comments, since my feedback is ultimately opinionated as well.

stevehipwell

This is looking like a really useful resource for contributions. I've added some comments/suggestions.

stevehipwell · 2026-05-13T08:44:04Z

+Request body types (for POST/PUT/PATCH requests) are named after the method
+they modify, with the suffix `Options`:


Should request body types not use the Request suffix? The use of Options here is overloaded and it looks to me like the current usage is split roughly even across the wtwo patterns.

stevehipwell · 2026-05-13T08:45:11Z

+- `Create` - Create a new resource
+- `Update` - Update an existing resource
+- `Delete` - Delete a resource
+- `Edit` - Edit an existing resource (alternative to Update)


I don't think we should have Edit as an "alternative", the GitHub API isn't consistent but that doesn't mean we can't make this SDK consistent (as far as is supported by the API).

stevehipwell · 2026-05-13T08:46:36Z

+Methods that operate on enterprise or organization resources include the scope
+in their name:


This comment doesn't line up with the example below it and also doesn't match the last agreed practice of NOT including the scope in the name where the scope is already defined by the service.

stevehipwell · 2026-05-13T08:48:50Z

+- `ListOptions` - For offset-based pagination (page/per_page)
+- `ListCursorOptions` - For cursor-based pagination


Do we want to move away from the use of some of the common structs that don't directly map to the API surface and instead start to add specific structs?

stevehipwell · 2026-05-13T08:59:00Z

+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"`
+}
+```


I don't think this comment is correct, for structs and time.Time using omitzero will omit the value from the payload; but for slices and maps it does the opposite and keeps empty ones and only omits them if they're nil. See the use in RepositoryRuleset.

stevehipwell · 2026-05-13T09:00:06Z

+
+### Pagination
+
+The go-github library supports two types of pagination:


Is this still correct, I thought it was fuzzier?

stevehipwell · 2026-05-13T09:00:57Z

+}
+```
+
+3. **Use iterators** for list methods that support pagination. The library


We probably out to mention the custom pagination configuration for non-standard pagination (e.g. commit files).

stevehipwell · 2026-05-13T09:01:47Z

+type Repository struct {
+    ID *int64 `json:"id,omitempty"`
+    // ...
+}
+
+type User struct {
+    ID *int64 `json:"id,omitempty"`
+    // ...
+}


It might be better to choose an example where the ID isn't a pointer.

stevehipwell · 2026-05-13T09:03:41Z

+    CreatedAt *Timestamp `json:"created_at,omitempty"`
+    UpdatedAt *Timestamp `json:"updated_at,omitempty"`
+    PushedAt  *Timestamp `json:"pushed_at,omitempty"`


Do these still need to be pointers or can we use omitzero? If so should we?

stevehipwell · 2026-05-13T09:04:29Z

+Boolean fields are always `*bool` (pointer to bool) to distinguish between
+`false` and `not set`:


If the boolean is required I'm not sure I see the value in this?

Not-Dhananjay-Mishra · 2026-05-13T09:45:19Z

+- `Delete` - Delete a resource
+- `Edit` - Edit an existing resource (alternative to Update)
+
+Common local variable names:


We can add opts in this list too.
As opts is more commonly used throughout the repository #3887

Not-Dhananjay-Mishra · 2026-05-13T10:53:09Z

+    Committer *CommitAuthor `json:"committer,omitempty"`
+}
+```
+


In my opinion it is worth mentioning about the use of value parameters instead of pointers where appropriate #3644

Not-Dhananjay-Mishra · 2026-05-13T11:08:33Z

+### JSON Tags for Request Bodies
+
+When defining structs that represent a request body (sent via POST/PUT/PATCH):


We have similar kind of rules around pointer usage and omitempty/omitzero for response bodies as well, so I think it would be worth mentioning them too.
Maybe we can combine the request and response body sections into a single section or should we make separate section?

alexandear · 2026-05-14T10:49:04Z

@jlaportebot, are you planning to continue working on this?

jlaportebot · 2026-05-17T07:19:06Z

@alexandear Yes, I'm planning to continue working on this. I'll address the requested changes around excessive formatting, redundant information, and duplicated content shortly. Thanks for the patience.

- Move Code Comments section under Code Guidelines - Remove 'Other notes on code organization' (merged into File Names) - Remove Linter Rules subsection (duplicates other sections) - Add Request suffix as alternative for request body types - Remove Edit from method naming (SDK should be consistent with Update) - Fix Enterprise/Org scoped methods description (scope from service type) - Correct omitzero behavior for slices vs structs - Add response body JSON tag guidance - Mention custom pagination for non-standard APIs - Add opts to common variable names - Add value parameters guidance - Use tabs in Go code examples - Remove excessive bold formatting

jlaportebot · 2026-05-17T10:29:37Z

Addressed all review feedback in a new commit (6dc10b8):

Moved Code Comments section under Code Guidelines per @alexandear
Removed 'Other notes on code organization' section — merged the GitHub API docs link into the File Names subsection per @alexandear and @munlicode
Removed Linter Rules subsection per @alexandear
Added Request suffix as an alternative naming pattern for request body types per @stevehipwell
Removed Edit from method naming — the SDK should be consistent with Update per @stevehipwell
Rewrote the Enterprise/Organization scoped methods section — methods don't need scope in the name since the service type already defines it per @stevehipwell
Corrected the omitzero description: for structs and time.Time, omitzero omits empty values; for slices and maps, omitzero keeps empty (non-nil) values and only omits nil per @stevehipwell. Added reference to RepositoryRuleset for examples.
Added response body JSON tag guidance per @Not-Dhananjay-Mishra
Added custom pagination configuration note for non-standard APIs (e.g., commit files) per @stevehipwell
Added opts to the common local variable names list per @Not-Dhananjay-Mishra
Added value parameters guidance (prefer value types over pointers where appropriate) per @Not-Dhananjay-Mishra (Refactor codebase to use value parameters instead of pointers where appropriate #3644)
Used tabs in Go code examples per @alexandear
Removed excessive bold formatting throughout the Code Guidelines section per @alexandear
Added non-pointer int64 example for ID fields in request bodies per @stevehipwell
Added boolean field guidance: non-pointer bool for required fields, *bool with omitempty for optional, *bool with omitzero when distinguishing false from not set per @stevehipwell
Noted that addOptions handles nil options so explicit nil checks are unnecessary per @alexandear

Please take another look — happy to make further adjustments.

- Move Code Comments under Code Guidelines section - Remove duplicated content (Code Organization, Linters) - Use TABs instead of spaces in code examples - Remove excessive numbered list formatting - Fix omitzero description for slices/maps per stevehipwell - Use non-pointer ID in example per stevehipwell - Add opts to common variable names per Not-Dhananjay-Mishra - Add Value vs Pointer Parameters section per google#3644 - Fix method naming section to match scoped method convention - Add custom pagination note per stevehipwell - Consolidate request/response body JSON tag guidelines - Fix boolean field guidance (use omitzero for optional bools) - Remove Edit from method name list, keep consistent with API

jlaportebot · 2026-05-17T17:37:46Z

Thanks for the detailed reviews! I've pushed a comprehensive update addressing the feedback:

@alexandear:

Moved Code Comments under Code Guidelines section
Removed duplicated content (Other notes on code organization section, linters section)
Used TABs instead of spaces in all code examples
Removed excessive numbered list formatting

@stevehipwell:

Fixed omitzero description: clarified that for slices/maps it keeps empty (non-nil) values and only omits nil values, with reference to RepositoryRuleset
Used non-pointer int64 for ID field in request body example
Added note about custom pagination for non-standard APIs (e.g., commit files)
Fixed boolean field guidance: use *bool with omitzero for optional fields where you need to distinguish between false and "not set"
Removed Edit from method name list to keep SDK consistent
Fixed method naming section to match the scoped method convention (method name should not repeat scope)

@Not-Dhananjay-Mishra:

Added opts to common variable names
Added Value vs Pointer Parameters section with reference to Refactor codebase to use value parameters instead of pointers where appropriate #3644

Other improvements:

Consolidated request/response body JSON tag guidelines
Removed redundant number formatting throughout
Streamlined the overall structure

I'd appreciate another round of review. Happy to make further adjustments.

gmlewis approved these changes May 8, 2026

View reviewed changes

alexandear suggested changes May 10, 2026

View reviewed changes

munlicode suggested changes May 13, 2026

View reviewed changes

stevehipwell suggested changes May 13, 2026

View reviewed changes

Not-Dhananjay-Mishra suggested changes May 13, 2026

View reviewed changes

munlicode approved these changes May 17, 2026

View reviewed changes


		[Go Doc Comments]: https://go.dev/doc/comment

		## Code Guidelines


		When defining structs that represent a request body (sent via POST/PUT/PATCH):

		1. Required fields should be non-pointer types without `omitempty`:

	1. Required fields should be non-pointer types without `omitempty`:
	1. Required fields should be non-pointer types without `omitempty`:

		Request body types (for POST/PUT/PATCH requests) are named after the method
		they modify, with the suffix `Options`:

		Methods that operate on enterprise or organization resources include the scope
		in their name:

		- `ListOptions` - For offset-based pagination (page/per_page)
		- `ListCursorOptions` - For cursor-based pagination


		### Pagination

		The go-github library supports two types of pagination:

		Boolean fields are always `*bool` (pointer to bool) to distinguish between
		`false` and `not set`:

		### JSON Tags for Request Bodies

		When defining structs that represent a request body (sent via POST/PUT/PATCH):

Conversation

jlaportebot commented May 8, 2026

Summary

Changes

Benefits

Testing

Uh oh!

gmlewis left a comment

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented May 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

alexandear left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexandear commented May 12, 2026

Uh oh!

stevehipwell commented May 12, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexandear commented May 13, 2026

Uh oh!

stevehipwell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexandear commented May 14, 2026

Uh oh!

jlaportebot commented May 17, 2026

Uh oh!

jlaportebot commented May 17, 2026

Uh oh!

jlaportebot commented May 17, 2026

Uh oh!

Reviewers

Assignees

codecov Bot commented May 8, 2026 •

edited

Loading