Repository navigation
Conversation
7e03fbd to
7b1aa83
Compare
7b1aa83 to
e44b749
Compare
f4e0045 to
16b112c
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 6 files. |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Title check | |
The title clearly and concisely describes the main change: optional scalar fields in generated Go output now use pointers. |
| Description check | |
The description directly explains the pointer conversion, its rationale, collection behavior, generated goldens, and consumer impact. |
Comment @coderabbitai help to get the list of available commands.
|
timestamps don't need to be pointers, #36 went with |
|
This has been discussed, I think the best way to address it will be to have meta hints on the JTD for the omit that the type wants |
Optional properties holding an object rendered as a value type, so an absent object and one whose fields all happen to be zero decoded identically. Consumers had no way to tell them apart and resorted to guessing from a field (if x.Order.ID != ""), which is wrong whenever the zero value is legitimate. Optional properties and discriminator forms now render as pointers. Adds a ref-to-struct case to the optional-refs fixture, which previously only covered scalars and collections. Optional string, bool, float64 and timestamp fields are ambiguous for the same reason but are left alone: changing them affects every generated client, so it wants its own decision.
Optional scalars rendered as value types, so absence and a legitimate zero
value decoded identically. Timestamps were the worst case: omitzero only
affects marshalling, so an absent timestamp still decoded to time.Time{} and
read downstream as a real instant in year 1 rather than as missing.
Optional string, bool, float and timestamp properties now render as pointers,
matching the treatment integers already had. Collections keep omitzero, since
a nil slice or map already distinguishes absence.
This changes the shape of every generated Go client, so it is split from the
object-field fix it stacks on: consumers of optional scalars need a pass to
dereference them.
e44b749 to
89c9ba4
Compare
| NullableTags *[]string `json:"nullableTags,omitempty"` | ||
| Scores map[string]int32 `json:"scores,omitzero"` | ||
| SeenAt time.Time `json:"seenAt,omitzero"` | ||
| SeenAt *time.Time `json:"seenAt,omitempty"` |
There was a problem hiding this comment.
I really think omitzero is just much more ergonomic here
It is worth keeping in mind that making something a pointer forces you to either check it everywhere or pull it out of the struct, so if we can avoid it and rely on validation it just becomes much simpler.
Anyway, I think @smithamax has a design in mind for this as I've mentioned, I'll defer to him
Internal change only
Optional string, bool, float, enum, timestamp and object properties in generated Go clients now render as pointers, as optional integers already did. As value types an absent field and a legitimate zero decode the same way: an absent timestamp decodes to
time.Time{}, and an absent object is only detectable by guessing from a field likeid != "". Collections keepomitzero, since a nil slice or map already means absent.This changes the shape of every generated Go client. Consumers of optional scalars and objects need a pass to dereference them.
Screenshots / Videos
Updated goldens under
lokerpc/codegen/testdata/(optional-collections,optional-refs) show optional scalars, enums and objects as pointers withomitempty, while collections keepomitzero.I have:
all the above must be ticked