docs: Extend CONTRIBUTING.md with code guidelines#4299
Conversation
…rays (google#3891) BREAKING CHANGE: `UpdateRepositoryRulesetClearBypassActor`, `UpdateRepositoryRulesetClearBypassActor`, `UpdateRulesetClearBypassActor`, and `UpdateRulesetNoBypassActor` have been removed as they are no longer needed.
…en branch is not protected (google#3896)
BREAKING CHANGE: `CustomProperty.DefaultValue` is now type `any` and `.ValueType` is now type `PropertyValueType`.
…4243) BREAKING CHANGE: App installation methods are renamed from `Find*` to `Get*`.
|
@jlaportebot Why was the previous PR #4202 closed? It has a lot of useful comments. |
So I'm of the opinion (derived from empirical evidence) that most contributors do not bother to read CONTRIBUTING.md, unfortunately. I'm also of the opinion that one of the reasons might be "TL;DR" (it is too enormous). I understand the arguments about making it super detailed, accounting for as many scenarios as possible that a contributor might encounter while writing a PR so that an AI Agent can slurp it all in and do a better job, but at the same time I don't want to drive away humans from contributing just because it appears to be a daunting experience before they even get started. Therefore, if one of our esteemed and greatly appreciated REVIEWERS wishes to improve it, that sounds wonderful to me. If others see gaps that they feel need filling, I think it is fine for we, as fellow contributors, to review and incorporate if we feel there is value. I hope this makes sense. |
|
@gmlewis @alexandear Thanks for the feedback — I appreciate the perspective on keeping CONTRIBUTING.md concise and human-friendly. On the CLA: I've signed the Google CLA (via https://cla.developers.google.com). The "waiting for signed CLA" label may be stale — happy to have a maintainer remove it. On length & approach: You're right that a massive CONTRIBUTING.md deters contributors. My goal isn't to document every edge case for AI agents — it's to capture the actual patterns reviewers keep flagging in PRs, so contributors (human or not) hit fewer review cycles. Feedback incorporated from #4202 review:
What's in this PR now (292 additions, 34 deletions — net +258 lines):
Request: Could a reviewer (@gmlewis or @alexandear) take a look? I've tried to keep it focused on patterns that actually come up in reviews rather than exhaustive documentation. If there are sections you'd prefer to cut or move to a separate wiki/doc, happy to adjust. |
|
Updated the branch with fixes for the gofmt issues @stevehipwell caught:
The branch is now at . Checks should re-run. @gmlewis @alexandear — when you have a moment, could one of you review? I've tried to keep the addition focused on patterns that actually come up in reviews (per our discussion on keeping it concise). Happy to trim further if any sections feel like over-documentation. |
2a6134d to
863cc19
Compare
|
Updated the PR with a single squashed commit that incorporates all feedback:
Net change: ~258 lines added (down from ~326). Focused on patterns that actually come up in reviews rather than exhaustive documentation. Ready for review whenever you have cycles! |
Unfortunately, the signed CLA check still fails and we cannot move forward without that item being taken care of. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4299 +/- ##
==========================================
+ Coverage 97.48% 97.50% +0.01%
==========================================
Files 191 193 +2
Lines 19206 19451 +245
==========================================
+ Hits 18723 18965 +242
- Misses 268 269 +1
- Partials 215 217 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@gmlewis @alexandear I've re-signed the Google CLA at https://cla.developers.google.com — it now shows a signed timestamp. The CLA verification system may take a bit to refresh the check on this PR. If there's anything else you'd like me to adjust in the CONTRIBUTING.md updates (trim sections, reword anything, move content to a separate doc, etc.), just let me know. Happy to iterate further before review. |
Nope, sorry. I just performed a rescan by clicking on the "cla/google" link and then following the instructions, and the rescan is instantaneous, and still fails. We unfortunately still cannot move forward with this PR until this is fixed. One particularly finicky thing about this system is that some commits may be attributed to different email addresses, and ALL email addresses must have a signed CLA. Sometimes it is possible to fix all the commits and then perform a force push (which is the only time I recommend a force push) so that all the commits use the same email address. I hope this helps. |
Add detailed code style and documentation guidelines including: - Method documentation conventions with godoc examples - API endpoint documentation patterns - Error handling guidelines - Type naming and struct conventions - Testing requirements and patterns
- 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
863cc19 to
3579492
Compare
Summary
Extends CONTRIBUTING.md with comprehensive code guidelines for contributors, including:
Addresses feedback from maintainers on the initial submission.
Changes