basecamp / basecamp/basecamp-sdk
Decide whether go/pkg/types belongs inside the timestamp pairing walk
- Dominant language
- Go
- Stars
- 49
- Forks
- 12
- Avg merge
- 20h 47m
- Merged PRs (30d)
- 89
Description
Surfaced while closing #722 in #790; predates that PR and is not changed by it.
`go/pkg/basecamp/optional_timestamps_test.go` reads `go/pkg/types` for exactly one purpose: `timeWrapperNames` parses it to **discover wrapper names** — any package-level struct embedding a value `time.Time` (today `FlexibleTime`), plus `Date` hard-coded — so that fields typed `types.FlexibleTime` / `types.Date` count as timestamps when the wrapper and generated sources are walked.
`collectTimestampFields` itself never walks `go/pkg/types`. Its two inputs are `go/pkg/basecamp/*.go` (non-test) and `go/pkg/generated/client.gen.go`. Consequences:
- `FlexibleTime`'s own `time.Time` embed is not subject to the embed guard #790 added, and `TestAllowedEmbedsMatchesCorpus` walks the same two sets, so it is not on — and does not need to be on — `allowedEmbeds`. If the walk is ever widened to `go/pkg/types`, that embed becomes the third entry.
- `types.FlexibleTime` / `types.Date` have no generated counterpart to pair with, so the `(struct, json key)` parity test could say nothing about them even if it walked the package. Their optionality contract is held behaviourally by `TestNamedTimeWrappersMarshalZeroAsNull` instead.
**The question.** Should `go/pkg/types` be inside the timestamp pairing at all, or is its current position — a name-discovery source only, with its own behaviour pinned by a marshal test — the right one? Two honest answers:
1. **Leave it out, and say so in the file.** The pairing compares hand-written wrappers against generated types; `pkg/types` is neither. The marshal test is the right instrument for a type with no counterpart. What's owed is a sentence in `optional_timestamps_test.go` naming `go/pkg/types` as deliberately outside the walk, so the next reader doesn't assume coverage it doesn't have.
2. **Walk it for the embed guard only.** The guard (not the pairing) is about anonymous embeds hiding promoted keys, and `FlexibleTime` embeds `time.Time`. Adding the package to the guard's inputs would put that embed under the same human-cleared allowlist as `FlexTime`'s — one more entry, zero new semantics.
Leaning (1): the embed inside `FlexibleTime` is the wrapper's *own* definition, the same shape `FlexTime` clears for the same reason, and the marshal test already asserts the behaviour the pairing would be inferring. Related: #633 (the `FlexibleTime` marshal asymmetry — a behaviour question on the same type, and a reason to prefer behaviour tests over structural ones here).
Contributor guide
Research direction
Start with go/pkg/basecamp/optional_timestamps_test.go and read collectTimestampFields, timeWrapperNames, TestAllowedEmbedsMatchesCorpus, and TestNamedTimeWrappersMarshalZeroAsNull. Compare the current walk inputs with the two alternatives in the issue and related issues #722, #790, and #633. Done means the scope decision is recorded in the test file or the guard inputs are updated, with the relevant tests passing.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- testing
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100