docs: trim comments to non-obvious WHY only#2
Merged
Conversation
Remove docblocks that restated the type name (WHAT) or duplicated a governing ADR, and strip ADR/tooling-enforced clauses (e.g. worker-mode call-time resolution, already covered by ADR-0007 and the Igor CI check). Keep only comments carrying non-obvious WHY: the AbstractAuditEvent extension seam, AuditQuery AND-combination, RecordReader actors() rationale, NewAuditRecord write/read disambiguation, and the resolver role lines.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Reviewed every comment in
src/against the eight ADRs,deptrac.yaml, and the Igor worker-safety check (ADR-0007), applying a strict WHY-only standard: a comment earns its place only if it carries non-obvious rationale not already documented elsewhere.AuditEvent,Changeset,FieldChange,RenderLine,RenderPayload,Subject,AuditActor,AuditEntry,AuditPage,AuditRecord,Actor,DefaultRecorder,IdentityGenerator,Origin,RecordStore,Recorder.ActorResolver/OriginResolver— dropped the "live, at call time (worker mode)" clauses (ADR-0007 + Igor CI).SubjectLabeller— kept the snapshot-survives-deletion WHY, dropped the redundant null-return note.AbstractAuditEvent— kept the extension-seam guidance, dropped the ADR-0002 "frozen render payload" echo.AuditQuery— kept "filters combine with AND" (non-obvious), dropped the name restatement.RecordReader— kept theactors()-for-filter-lists rationale.NewAuditRecord— kept the write-input vsRecord\AuditRecorddisambiguation.All
@param/@return/@vartype annotations preserved (PHPStan needs them).Note
With strict WHY-only, the central domain interfaces (
AuditEvent,Recorder,AuditRecord) now carry no class-level orientation — that context lives in the ADRs. If this hurts source-level discoverability for consumers, a one-line role on public types is the middle ground.Test plan
composer cs-check— cleancomposer phpstan— 0 errorscomposer test— 18 tests, 51 assertions pass