From a2cde1c2d1c455a9e40c866e2523bef263cdffd7 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 3 Aug 2026 22:34:20 -0700 Subject: [PATCH] Go: export Ptr and Deref so consumers stop hand-rolling them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit v0.13.0 pointerized the optional fields across the Go surface (#560, #615, #632) and shipped no way to build a pointer. `ptr[T any]` has been sitting unexported in helpers.go the whole time, and go/README.md said nothing about pointer fields, so every consumer hitting the migration writes their own generic helper first. This SDK's own test suite is the proof: schedules_test.go and test_helpers_test.go hand-rolled strPtr, boolPtr, idsPtr and intPtr rather than reach for the unexported one. One generic Ptr rather than AWS-style typed constructors. The optional fields span *string, *bool, *int, *int32, *int64, *time.Time and *[]int64 — a typed set would need six names and still not cover ParticipantIDs *[]int64, where a pointer to an empty slice is what removes every participant. Deref covers the read direction, which is the more dangerous half. Go auto-dereferences a value-receiver method call, so hc.UpdatedAt.IsZero() still compiles against *time.Time and panics at run time on a chart that has never moved. Deref is total: the zero value on nil. The unexported ptr and deref stay as the internal vocabulary at hundreds of conversion sites, but now forward to the exported pair, so the contract callers get cannot drift from the one this package relies on. Additive only: no existing exported signature changes. --- go/README.md | 31 +++++ go/pkg/basecamp/doc.go | 21 ++++ go/pkg/basecamp/example_test.go | 42 +++++++ go/pkg/basecamp/helpers.go | 16 ++- go/pkg/basecamp/pointers.go | 56 +++++++++ go/pkg/basecamp/pointers_test.go | 199 +++++++++++++++++++++++++++++++ 6 files changed, 359 insertions(+), 6 deletions(-) create mode 100644 go/pkg/basecamp/pointers.go create mode 100644 go/pkg/basecamp/pointers_test.go diff --git a/go/README.md b/go/README.md index e304bdaaf1..c34def8de8 100644 --- a/go/README.md +++ b/go/README.md @@ -387,6 +387,37 @@ cfg.LoadConfigFromEnv() cfg, err := basecamp.LoadConfig("/path/to/config.json") ``` +## Optional Fields + +Optional fields are pointers, so that "not addressed" stays distinguishable from +a value. Nil omits the field; a non-nil pointer sends the value verbatim, +including the zero value. `basecamp.Ptr` builds one for any type: + +```go +entry, err := account.Schedules().UpdateEntry(ctx, entryID, &basecamp.UpdateScheduleEntryRequest{ + Summary: basecamp.Ptr("Kickoff, moved"), + AllDay: basecamp.Ptr(false), // an explicit false, not "unset" + ParticipantIDs: basecamp.Ptr([]int64{}), // an explicit empty list: remove everyone + // Description stays nil, so the entry's description is left alone. +}) +``` + +Reading one is the half that fails quietly: Go auto-dereferences a value-receiver +method call, so `hc.UpdatedAt.IsZero()` compiles against a `*time.Time` and +panics at run time on a chart that has never moved. Nil-check it, or let +`basecamp.Deref` return the zero value for you: + +```go +hc, err := account.HillCharts().Get(ctx, todosetID) +if updated := basecamp.Deref(hc.UpdatedAt); !updated.IsZero() { + fmt.Println("last moved", updated) +} +``` + +Collapsing absence to the zero value is only safe where the caller cannot tell +the two apart. Where the difference carries meaning — a string the server really +sent as empty versus a field it omitted — compare against nil instead. + ## API Coverage ### Projects & Organization diff --git a/go/pkg/basecamp/doc.go b/go/pkg/basecamp/doc.go index 5181a6eb7f..4793e98dcf 100644 --- a/go/pkg/basecamp/doc.go +++ b/go/pkg/basecamp/doc.go @@ -66,6 +66,27 @@ // - [AccountClient.Attachments] - File attachments // - [Client.Authorization] - Account-agnostic authorization info // +// # Optional Fields +// +// Optional fields are pointers, so that "not addressed" stays distinguishable +// from a value. Nil omits the field; a non-nil pointer sends the value +// verbatim, including the zero value. [Ptr] builds one for any type: +// +// entry, err := account.Schedules().UpdateEntry(ctx, entryID, &basecamp.UpdateScheduleEntryRequest{ +// Summary: basecamp.Ptr("Kickoff, moved"), +// AllDay: basecamp.Ptr(false), // an explicit false, not "unset" +// ParticipantIDs: basecamp.Ptr([]int64{}), // an explicit empty list: remove everyone +// }) +// +// Reading one is the half that fails quietly: Go auto-dereferences a +// value-receiver method call, so hc.UpdatedAt.IsZero() compiles against a +// *time.Time and panics at run time on a chart that has never moved. Nil-check +// it, or let [Deref] return the zero value for you: +// +// if updated := basecamp.Deref(hc.UpdatedAt); !updated.IsZero() { +// fmt.Println("last moved", updated) +// } +// // # Working with Projects // // List all projects: diff --git a/go/pkg/basecamp/example_test.go b/go/pkg/basecamp/example_test.go index 6d8ba919a0..3bb82b1b5e 100644 --- a/go/pkg/basecamp/example_test.go +++ b/go/pkg/basecamp/example_test.go @@ -2,12 +2,14 @@ package basecamp_test import ( "context" + "encoding/json" "errors" "fmt" "log" "log/slog" "net/http" "os" + "time" "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" ) @@ -403,3 +405,43 @@ func ExampleCampfiresService_CreateLine() { fmt.Printf("Message posted: %s\n", line.Content) } + +func ExamplePtr() { + // Optional request fields are pointers so that "not addressed" stays + // distinguishable from a value. Ptr sets one; leaving it nil omits it. + req := &basecamp.UpdateScheduleEntryRequest{ + Summary: basecamp.Ptr("Kickoff, moved"), + AllDay: basecamp.Ptr(false), // an explicit false, not "unset" + ParticipantIDs: basecamp.Ptr([]int64{}), // an explicit empty list: remove everyone + // Description stays nil, so the entry's description is left alone. + } + + body, err := json.Marshal(req) + if err != nil { + log.Fatal(err) + } + + fmt.Println(string(body)) + // Output: {"summary":"Kickoff, moved","all_day":false,"participant_ids":[]} +} + +func ExampleDeref() { + // A hill chart that has never moved omits updated_at, so UpdatedAt is nil. + // Calling hc.UpdatedAt.IsZero() directly compiles and panics; Deref is total. + never := &basecamp.HillChart{Enabled: true} + moved := &basecamp.HillChart{ + Enabled: true, + UpdatedAt: basecamp.Ptr(time.Date(2026, 8, 3, 9, 30, 0, 0, time.UTC)), + } + + for _, hc := range []*basecamp.HillChart{never, moved} { + if updated := basecamp.Deref(hc.UpdatedAt); updated.IsZero() { + fmt.Println("never moved") + } else { + fmt.Println("last moved", updated.Format(time.RFC3339)) + } + } + // Output: + // never moved + // last moved 2026-08-03T09:30:00Z +} diff --git a/go/pkg/basecamp/helpers.go b/go/pkg/basecamp/helpers.go index 3042f64048..ff07b2c752 100644 --- a/go/pkg/basecamp/helpers.go +++ b/go/pkg/basecamp/helpers.go @@ -289,12 +289,13 @@ func truncate(s string) string { // deref safely dereferences an optional-field pointer, returning the zero // value when the field was absent. +// +// The internal spelling of the exported [Deref], kept as the vocabulary the +// hundreds of existing conversion sites already read in. Forwarding rather than +// reimplementing keeps one definition, so the contract callers get cannot drift +// from the contract this package relies on. func deref[T any](p *T) T { - if p == nil { - var zero T - return zero - } - return *p + return Deref(p) } // omitzero converts a value-typed wrapper option to a generated request's @@ -310,8 +311,11 @@ func omitzero[T comparable](v T) *T { // ptr returns a pointer to v, for optional fields where the value — zero // included — must be sent. +// +// The internal spelling of the exported [Ptr]; see deref for why it forwards +// rather than reimplements. func ptr[T any](v T) *T { - return &v + return Ptr(v) } // intPtrFrom converts an optional generated int32 pointer to the SDK's *int, diff --git a/go/pkg/basecamp/pointers.go b/go/pkg/basecamp/pointers.go new file mode 100644 index 0000000000..c22c18c80e --- /dev/null +++ b/go/pkg/basecamp/pointers.go @@ -0,0 +1,56 @@ +package basecamp + +// Optional fields on this SDK's request and response types are pointers, so +// that absence stays distinguishable from a value (SPEC.md §10): nil means the +// field was not addressed, and a non-nil pointer means this exact value — the +// zero value included. Go has no literal syntax for the address of a constant, +// so writing to those fields otherwise costs a named variable each, and reading +// from them silently compiles into a nil dereference. Ptr and Deref are the two +// halves of that round trip. + +// Ptr returns a pointer to v, for setting an optional field. +// +// A nil optional field is omitted from the request; a non-nil one is sent +// verbatim. Ptr therefore always allocates, and never collapses false or "" to +// nil — sending an explicit zero is the whole reason these fields are pointers. +// +// Being generic over every type, one helper covers the scalar fields and the +// pointer-to-slice fields alike: +// +// entry, err := account.Schedules().UpdateEntry(ctx, entryID, &basecamp.UpdateScheduleEntryRequest{ +// Summary: basecamp.Ptr("Kickoff, moved"), +// AllDay: basecamp.Ptr(false), // an explicit false, not "unset" +// ParticipantIDs: basecamp.Ptr([]int64{}), // an explicit empty list: remove everyone +// }) +// +// T is inferred from the argument, so a field whose type is not an untyped +// literal's default needs the conversion written out: basecamp.Ptr(int32(5)) +// for an *int32 field, not basecamp.Ptr(5). +func Ptr[T any](v T) *T { + return &v +} + +// Deref returns the value p points at, or the zero value of T when p is nil. +// +// Reading an optional field is the half that fails quietly. Go auto-dereferences +// a value-receiver method call, so hc.UpdatedAt.IsZero() still compiles against +// a *time.Time and panics at run time on a hill chart that has never moved. +// Deref is total, and makes the absent case an ordinary value: +// +// hc, err := account.HillCharts().Get(ctx, todosetID) +// // ... +// if updated := basecamp.Deref(hc.UpdatedAt); !updated.IsZero() { +// fmt.Println("last moved", updated) +// } +// +// Collapsing absence to the zero value is only correct where the caller cannot +// tell the two apart anyway. Where the difference carries meaning — a string the +// server really sent as empty versus a field it omitted — compare against nil +// instead. +func Deref[T any](p *T) T { + if p == nil { + var zero T + return zero + } + return *p +} diff --git a/go/pkg/basecamp/pointers_test.go b/go/pkg/basecamp/pointers_test.go new file mode 100644 index 0000000000..7714ab6627 --- /dev/null +++ b/go/pkg/basecamp/pointers_test.go @@ -0,0 +1,199 @@ +package basecamp + +import ( + "testing" + "time" +) + +// Ptr and Deref are the exported halves of the optional-pointer contract +// (SPEC.md §10), and both have a failure mode that a plausible "simplification" +// would introduce silently. Ptr must allocate unconditionally — collapsing a +// zero value to nil would turn "set this to false" into "leave it alone", which +// is the distinction the pointer exists to carry. Deref must be total — the +// obvious one-liner `return *p` compiles, passes every non-nil test, and panics +// on exactly the absent field it was reached for. + +// assertPtrRoundTrip pins Ptr's whole contract for one comparable type: a +// non-nil pointer, addressing that value. +func assertPtrRoundTrip[T comparable](t *testing.T, v T) { + t.Helper() + got := Ptr(v) + if got == nil { + t.Fatalf("Ptr(%v) = nil, want a non-nil pointer (a zero value must still reach the wire)", v) + } + if *got != v { + t.Errorf("*Ptr(%v) = %v, want %v", v, *got, v) + } +} + +func TestPtr_AddressesTheValueIncludingZero(t *testing.T) { + tests := []struct { + name string + run func(t *testing.T) + }{ + {"string zero", func(t *testing.T) { assertPtrRoundTrip(t, "") }}, + {"string value", func(t *testing.T) { assertPtrRoundTrip(t, "Kickoff, moved") }}, + {"bool false", func(t *testing.T) { assertPtrRoundTrip(t, false) }}, + {"bool true", func(t *testing.T) { assertPtrRoundTrip(t, true) }}, + {"int zero", func(t *testing.T) { assertPtrRoundTrip(t, 0) }}, + {"int32", func(t *testing.T) { assertPtrRoundTrip(t, int32(5)) }}, + {"int64", func(t *testing.T) { assertPtrRoundTrip(t, int64(1069479400)) }}, + {"time zero", func(t *testing.T) { assertPtrRoundTrip(t, time.Time{}) }}, + {"time value", func(t *testing.T) { + assertPtrRoundTrip(t, time.Date(2026, 8, 3, 9, 30, 0, 0, time.UTC)) + }}, + } + for _, tt := range tests { + t.Run(tt.name, tt.run) + } +} + +// The pointer-to-slice fields are the reason this is one generic helper rather +// than a set of typed constructors: UpdateScheduleEntryRequest.ParticipantIDs is +// *[]int64, where nil leaves the participants alone and a pointer to an empty +// slice removes everyone. Slices are not comparable, so this case cannot ride +// on assertPtrRoundTrip. +func TestPtr_AddressesSlicesIncludingTheEmptyOne(t *testing.T) { + tests := []struct { + name string + in []int64 + }{ + {"nil slice", nil}, + {"empty slice", []int64{}}, + {"populated slice", []int64{1069479400, 1069479401}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := Ptr(tt.in) + if got == nil { + t.Fatalf("Ptr(%v) = nil, want a non-nil pointer even for an empty or nil slice", tt.in) + } + if len(*got) != len(tt.in) { + t.Fatalf("len(*Ptr(%v)) = %d, want %d", tt.in, len(*got), len(tt.in)) + } + for i, want := range tt.in { + if (*got)[i] != want { + t.Errorf("(*Ptr(%v))[%d] = %d, want %d", tt.in, i, (*got)[i], want) + } + } + }) + } +} + +// Every call allocates. A shared address would alias every request built in a +// loop onto whatever the last iteration wrote. +func TestPtr_ReturnsADistinctPointerPerCall(t *testing.T) { + first, second := Ptr("Kickoff"), Ptr("Kickoff") + if first == second { + t.Fatal("two Ptr calls returned the same address; requests built in a loop would alias") + } + *first = "moved" + if *second != "Kickoff" { + t.Errorf("writing through one pointer changed the other: *second = %q, want %q", *second, "Kickoff") + } +} + +// The load-bearing case. `return *p` passes everything above and fails only +// here — by panicking, which is precisely the run-time break these tests exist +// to prevent (hc.UpdatedAt.IsZero() on an absent timestamp compiles fine). +func TestDeref_ReturnsTheZeroValueForNilRatherThanPanicking(t *testing.T) { + tests := []struct { + name string + run func(t *testing.T) + }{ + {"*string", func(t *testing.T) { + if got := Deref[string](nil); got != "" { + t.Errorf("Deref[string](nil) = %q, want %q", got, "") + } + }}, + {"*bool", func(t *testing.T) { + if got := Deref[bool](nil); got != false { + t.Errorf("Deref[bool](nil) = %v, want false", got) + } + }}, + {"*int", func(t *testing.T) { + if got := Deref[int](nil); got != 0 { + t.Errorf("Deref[int](nil) = %d, want 0", got) + } + }}, + {"*int64", func(t *testing.T) { + if got := Deref[int64](nil); got != 0 { + t.Errorf("Deref[int64](nil) = %d, want 0", got) + } + }}, + {"*time.Time", func(t *testing.T) { + // The exact shape the migration guide flags: HillChart.UpdatedAt is + // nil on a chart that has never moved. + var absent *time.Time + if got := Deref(absent); !got.IsZero() { + t.Errorf("Deref((*time.Time)(nil)) = %v, want the zero time", got) + } + }}, + {"*[]int64", func(t *testing.T) { + if got := Deref[[]int64](nil); got != nil { + t.Errorf("Deref[[]int64](nil) = %v, want nil", got) + } + }}, + } + for _, tt := range tests { + t.Run(tt.name, tt.run) + } +} + +func TestDeref_ReturnsThePointedAtValue(t *testing.T) { + tests := []struct { + name string + run func(t *testing.T) + }{ + {"string", func(t *testing.T) { + if got := Deref(Ptr("Kickoff")); got != "Kickoff" { + t.Errorf("Deref(Ptr(%q)) = %q, want %q", "Kickoff", got, "Kickoff") + } + }}, + {"explicit false survives", func(t *testing.T) { + // A present false and an absent field both read back as false; that + // collapse is Deref's documented cost, and the round trip through + // Ptr is what proves the value was carried, not manufactured. + if got := Deref(Ptr(false)); got != false { + t.Errorf("Deref(Ptr(false)) = %v, want false", got) + } + }}, + {"explicit empty string survives", func(t *testing.T) { + if got := Deref(Ptr("")); got != "" { + t.Errorf("Deref(Ptr(%q)) = %q, want %q", "", got, "") + } + }}, + {"time", func(t *testing.T) { + want := time.Date(2026, 8, 3, 9, 30, 0, 0, time.UTC) + if got := Deref(Ptr(want)); !got.Equal(want) { + t.Errorf("Deref(Ptr(%v)) = %v, want %v", want, got, want) + } + }}, + } + for _, tt := range tests { + t.Run(tt.name, tt.run) + } +} + +// The unexported spellings forward to the exported ones, so the contract the +// hundreds of internal conversion sites rely on is the same code consumers get. +// Pin that: a future reimplementation of either half has to keep them agreeing. +func TestUnexportedSpellingsForwardToTheExportedOnes(t *testing.T) { + t.Run("deref matches Deref on nil", func(t *testing.T) { + var absent *time.Time + if got, want := deref(absent), Deref(absent); !got.Equal(want) { + t.Errorf("deref(nil) = %v, Deref(nil) = %v; the two must not diverge", got, want) + } + }) + t.Run("deref matches Deref on a value", func(t *testing.T) { + p := Ptr("Kickoff") + if got, want := deref(p), Deref(p); got != want { + t.Errorf("deref(p) = %q, Deref(p) = %q; the two must not diverge", got, want) + } + }) + t.Run("ptr matches Ptr on a zero value", func(t *testing.T) { + if got, want := ptr(false), Ptr(false); got == nil || want == nil || *got != *want { + t.Error("ptr(false) and Ptr(false) must both address a non-nil false") + } + }) +}