Skip to content

docs: Extend CONTRIBUTING.md with code guidelines#4339

Merged
gmlewis merged 2 commits into
google:masterfrom
jlaportebot:docs/extend-contributing-guidelines
Jun 29, 2026
Merged

docs: Extend CONTRIBUTING.md with code guidelines#4339
gmlewis merged 2 commits into
google:masterfrom
jlaportebot:docs/extend-contributing-guidelines

Conversation

@jlaportebot

Copy link
Copy Markdown
Contributor

Extends CONTRIBUTING.md with comprehensive code guidelines for contributors, including:

  • Code style and formatting standards
  • Testing requirements and patterns
  • Documentation expectations
  • Commit message conventions
  • Code review guidelines

Addresses feedback from maintainers on the initial submission.

- Add Code Guidelines section with naming conventions, type conventions, JSON tags,
  URL tags, pagination, and common types
- Move Code Comments under Code Guidelines
- Remove duplicate content from Submitting a patch, Metadata, Scripts sections
- Fix gofmt formatting for all code snippets
- Incorporate reviewer feedback from google#4202 (alexandear, stevehipwell)
- Address gmlewis feedback on keeping CONTRIBUTING.md concise
@jlaportebot

Copy link
Copy Markdown
Contributor Author

CLA Fix Summary

Closed PR #4299 (and #4202) due to CLA check failures caused by:

  1. 2867 commits in the PR history — git filter-branch accidentally rewrote the entire repo history instead of just the PR branch
  2. Mixed email addresses — Author was jlaportebot@users.noreply.github.com but Committer was jlaportebot@gmail.com (CLA only signed for the noreply email)
  3. Invalid Co-authored-by trailer in original commit: Co-authored-by: review feedback from google/go-github maintainers (missing email in angle brackets)

What I did to fix:

  • Reset branch to upstream/master (clean base)
  • Cherry-picked the CONTRIBUTING.md changes as a single commit
  • Set both Author and Committer to jlaportebot@users.noreply.github.com (the email with signed Google CLA)
  • Removed the problematic Co-authored-by trailer

Result: PR #4339 now has 1 commit with clean CLA verification ✅

The CONTRIBUTING.md content is identical to what was in #4299 — all reviewer feedback incorporated (alexandear, stevehipwell, gmlewis). Ready for review!

@gmlewis gmlewis added the NeedsReview PR is awaiting a review before merging. label Jun 28, 2026
@codecov

codecov Bot commented Jun 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.50%. Comparing base (4b55679) to head (ed1e6bf).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4339   +/-   ##
=======================================
  Coverage   97.50%   97.50%           
=======================================
  Files         193      193           
  Lines       19451    19451           
=======================================
  Hits        18965    18965           
  Misses        269      269           
  Partials      217      217           

☔ View full report in Codecov by Harness.
📢 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.

@gmlewis gmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nit, otherwise LGTM.

Comment thread CONTRIBUTING.md Outdated
Comment on lines +174 to +185
...
}
```
And the returned type `Repository` will have comments like this:

```go
// Repository represents a GitHub repository.
type Repository struct {
ID *int64 `json:"id,omitempty"`
ID *int64 `json:"id,omitempty"`
NodeID *string `json:"node_id,omitempty"`
Owner *User `json:"owner,omitempty"`
// ...
Owner *User `json:"owner,omitempty"`
...

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion, these changes to the code sections are a regression and the new formatting looks worse than the original. I think that the changes to the new lines 174-185 should be reverted.

Revert the formatting changes to the code examples in the Code Comments
section to match the original style with tabs and // ... comments,
as requested in review feedback from gmlewis.
@jlaportebot

Copy link
Copy Markdown
Contributor Author

I've addressed the nit from the review - reverted the code formatting changes in the Code Comments examples to match the original style with tabs and // ... comments. The review was on the previous commit (130d1aa) and this is now on the new commit (ed1e6bf). The CLA check passes and all checks are green. Ready for re-review!

@gmlewis gmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stevehipwell stevehipwell left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I do still think that using the Repository struct in the example for a response object is unclear as it's two most significant fields (ID & Name) are pointers when they are required and by the coding standards logic should not be.

@gmlewis

gmlewis commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator

I do still think that using the Repository struct in the example for a response object is unclear as it's two most significant fields (ID & Name) are pointers when they are required and by the coding standards logic should not be.

This is an orthogonal issue. I believe it was originally done this way because, depending upon the endpoint where either *Repository or []*Repository was being returned, they were emperically found to not be populated by the GitHub server, and therefore were made pointers so that the client could more easily figure out if the JSON response actually included these fields.

Feel free to file a separate issue so that someone can investigate if this is no longer the case in any of the DotCom or current or legacy Enterprise versions of GitHub so that they can be changed.

@gmlewis gmlewis removed the NeedsReview PR is awaiting a review before merging. label Jun 29, 2026
@gmlewis gmlewis merged commit 4498e09 into google:master Jun 29, 2026
15 checks passed
@stevehipwell

Copy link
Copy Markdown
Contributor

This is an orthogonal issue. I believe it was originally done this way because, depending upon the endpoint where either *Repository or []*Repository was being returned, they were emperically found to not be populated by the GitHub server, and therefore were made pointers so that the client could more easily figure out if the JSON response actually included these fields.

Feel free to file a separate issue so that someone can investigate if this is no longer the case in any of the DotCom or current or legacy Enterprise versions of GitHub so that they can be changed.

@gmlewis I was very careful not to say that the Repository implementation wasn't correct, just that it made the example confusing compared to uisng a type where the identity fields weren't pointers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants