Introduce structured FBC types: MajorMinor and Date#29
Conversation
|
🤖 Finished Review · ✅ Success · Started 8:08 PM UTC · Completed 8:23 PM UTC |
ReviewFindingsHigh
Medium
Low
Previous runReviewFindingsHigh
Medium
Low
Labels: PR introduces new structured types and changes exported API surface, qualifying as an enhancement. |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Replace plain string fields in the FBC output types with structured types that enforce format invariants at construction time: - Version.Name: string → MajorMinor (uint64 Major/Minor, regex-validated) - Phase.StartDate/EndDate: string → *Date (wraps time.Time, YYYY-MM-DD) - Platform.Versions: []string → []MajorMinor Translation functions (translatePhase, translateVersion, newPackage) now return errors instead of silently passing through malformed data. Errors are collected with errors.Join (no fail-fast) and surfaced as validation failures. Empty timestamps and "N/A" translate to nil *Date (not an error). This enforces the FBC lifecycle contract — the schema that downstream consumers depend on — which is separate from the PLCC validation policy that governs data quality in the upstream source. PLCC validators check whether product lifecycle data meets content policy (tier requirements, date contiguity, release cadence, etc.); the FBC type layer ensures the output schema is well-formed by construction regardless of which PLCC validators are enabled. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
joelanford
force-pushed
the
jl/structured-fbc-types
branch
from
5497c67 to
0056b34
Compare
|
🤖 Finished Review · ✅ Success · Started 8:55 PM UTC · Completed 9:09 PM UTC |
| // newPackage translates a PLCC product without failing on bad data (unparseable | ||
| // timestamps become empty strings). [Translate] then runs a configurable filter | ||
| // pipeline that cleans the output (e.g., dropping incomplete phases). | ||
| // The package follows a two-phase design: strict translation followed by output cleanup. |
There was a problem hiding this comment.
[medium] architecture-shift-undocumented
The PR shifts the translation philosophy from lenient parsing to strict translation. While doc.go and AGENTS.md are updated, no formal design document captures the rationale, alternatives considered, or implications for the two-layer validation architecture.
Suggested fix: Consider documenting the design rationale more formally if the project adopts ADRs.
| // Version represents an operator version with its lifecycle phases and platform compatibility. | ||
| type Version struct { | ||
| Name string `json:"name"` | ||
| Name MajorMinor `json:"name"` |
There was a problem hiding this comment.
[medium] breaking-api-change
Exported type changes (Version.Name: string to MajorMinor, Phase.StartDate/EndDate: string to *Date, Platform.Versions: []string to []MajorMinor) are Go-level breaking changes. The module has no git tags or releases (pre-1.0), limiting external impact, but downstream Go importers would break at compile time.
Suggested fix: Acknowledge this as a pre-1.0 breaking change in the PR description.
|
|
||
| sort.Slice(pkg.Versions, func(i, j int) bool { | ||
| return compareMajorMinor(pkg.Versions[i].Name, pkg.Versions[j].Name) < 0 | ||
| slices.SortFunc(pkg.Versions, func(a, b Version) int { |
There was a problem hiding this comment.
[medium] breaking-json-output
Phase.StartDate and Phase.EndDate now use omitempty JSON tags. Nil dates are omitted from JSON output rather than appearing as empty strings. In practice, FilterIncompletePhases strips nil-date phases by default, so this mainly affects non-default filter pipelines.
Suggested fix: Remove omitempty from Phase date JSON tags to maintain backward compatibility, or document this as a known change.
| fv.Phases = append(fv.Phases, translatePhase(ph)) | ||
| fp, err := translatePhase(ph) | ||
| if err != nil { | ||
| errs = append(errs, fmt.Errorf("phase %q: %w", ph.Name, err)) |
There was a problem hiding this comment.
[medium] breaking-behavior
Translate() now rejects products with invalid version names (non-MAJOR.MINOR) or unparseable timestamps, whereas previously these passed through with degraded data. This is intentional per the PR description and redundant with existing PLCC validators in the normal pipeline.
Suggested fix: Document this behavior change and its interaction with existing PLCC validators.
| .PHONY: test | ||
| test: | ||
| go test -v ./... | ||
| go test -v -count 1 ./... |
There was a problem hiding this comment.
[low] scope-creep
Unrelated addition of -count 1 (disables test caching). Plausibly related to test data regeneration but not documented.
| errs = append(errs, fmt.Errorf("phase %q: %w", ph.Name, err)) | ||
| } else { | ||
| phases = append(phases, fp) | ||
| } |
There was a problem hiding this comment.
[low] error-handling
Nested errors.Join in newPackage produces multi-line entries in ValidationResult.Reasons, which may render poorly in structured JSON logs.
| // Compare returns a negative value if m < other, zero if equal, positive if m > other. | ||
| func (m MajorMinor) Compare(other MajorMinor) int { | ||
| if m.Major != other.Major { | ||
| if m.Major < other.Major { |
There was a problem hiding this comment.
[low] panic-usage
ParseMajorMinor includes a panic for an unreachable code path. The codebase has no other panic calls in production code.
| } | ||
| minor, err := strconv.ParseUint(matches[2], 10, 64) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid minor version %q: %w", matches[2], err) |
There was a problem hiding this comment.
[low] naming-inconsistency
MajorMinor uses uint64 for Major/Minor fields; this is the default return type of strconv.ParseUint but oversized for version numbers.
1 participant
Summary
MajorMinorandDatetypes that enforce format invariants at construction timeVersion.NameandPlatform.VersionsuseMajorMinor(regex-validated, no leading zeros);Phase.StartDate/EndDateuse*Date(nil = no date)errors.Join) instead of silently passing through malformed data; empty and "N/A" timestamps translate to nilTest plan
make testpassesGenerated with Claude Code