Skip to content

Decide whether go/pkg/types belongs inside the timestamp pairing walk #803

Description

@jeremy

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 Report an anonymous embed the timestamp walk cannot resolve, instead of skipping it #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).

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions