HYPERFLEET-1141 - docs: polish code-review standards#149
Conversation
…aden ERR-03 Add exception for read-only defer close (resp.Body.Close(), rows.Close()) where error is not actionable — MAY use bare defer without blank identifier. Resolves unresolved PR openshift-hyperfleet#147 nit from pnguyen44. Broaden ERR-03 from stdlib-only (http.Error, w.WriteHeader) to framework-agnostic principle covering any error-response write.
Restructure CONC-03 so Go 1.22+ skip is the first thing reviewers see, not buried in a Note after the MUST. Go 1.22 introduced per-iteration scoping that eliminates loop variable capture bugs.
…ches Document that all repos use test/integration/ directory, some additionally use //go:build integration build tags. Both patterns valid. Reviewers MUST NOT flag one approach when the project consistently uses the other.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR updates three HyperFleet Go code review standard documents. The concurrency standard clarifies that CONC-03 (loop variable capture) does not apply to Go 1.22+ projects and requires reviewers to verify Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
|
/retest |
| _ = resp.Body.Close() // best-effort cleanup; error already logged by HTTP client | ||
| ``` | ||
|
|
||
| **Exception — read-only defer close:** `defer` close calls on read-only resources (e.g., `resp.Body.Close()`, `rows.Close()`) where the error is not actionable MAY use bare `defer` without a blank identifier or comment. This is idiomatic Go for read-only cleanup. |
There was a problem hiding this comment.
"Read-only resources" is broad enough to be misapplied to db.Close() or conn.Close() where close errors signal data loss. Consider scoping to an explicit safe list (resp.Body.Close(), rows.Close()) and noting the exception does NOT apply to writable resources (files, DB connections, gRPC streams).
There was a problem hiding this comment.
Right, close errors on writable resources need to be handled.
…rion Reword defer close exception: criterion is whether resource was written to and close may flush, not resource type. Read-only file.Close() stays in exception, but writable file/DB handle close MUST be handled since it can signal data loss.
2 participants
Summary
Updates
error-handling.mdERR-01 with exception for read-onlydeferclose (resp.Body.Close(),rows.Close()) — resolves unresolved PR HYPERFLEET-1073 - docs: add code review standards index #147 nit from @pnguyen44Updates
error-handling.mdERR-03 from stdlib-only patterns to framework-agnostic principle ("anyerror-response write")
Updates
concurrency.mdCONC-03 to lead with Go 1.22+ version gate instead of burying it in a NoteUpdates
testing.mdTEST-03 exception and Enforcement section to document both integration test separationapproaches (directory-only and directory-plus-build-tag)
HYPERFLEET-1141
Test Plan
make test-allpassesmake lintpassesmake test-helm(if applicable)