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") + } + }) +}