[typeid] Remove NullableID in favor of sql.Null[TypeID], remove Must() #519
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Remove custom NullableID type in favor of Go's standard sql.Null[TypeID] - Update all tests to use sql.Null[TypeID] instead of NullableID - Update examples to demonstrate sql.Null[TypeID] usage - Add documentation recommending sql.Null[TypeID] for nullable columns - Maintain full backward compatibility for database operations
- Remove Must function from typeid.go - Add MustParse helper in shared_test.go for test usage - Update all test files to use MustGenerate for new TypeIDs - Update all test files to use MustParse for parsing TypeID strings - All tests now pass with the new helper functions
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR replaces the custom NullableID type with Go’s standard sql.Null[TypeID], removes the Must helper in the public API (introducing MustParse only for tests), and updates all tests, examples, and benchmarks accordingly.
- Swap
NullableIDforsql.Null[TypeID]in code, tests, and examples - Remove the public
Mustfunction and add a test‐onlyMustParsehelper - Update benchmarks to use
MustGenerate(though helper isn’t yet defined)
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| typeid/typeid-go/typeid.go | Removed the Must function |
| typeid/typeid-go/sql.go | Deleted NullableID type and methods; added usage note |
| typeid/typeid-go/sql_test.go | Replaced NullableID tests with sql.Null[TypeID] and MustParse |
| typeid/typeid-go/shared_test.go | Added MustParse helper for tests |
| typeid/typeid-go/examples_test.go | Updated examples to use sql.Null[TypeID] and MustParse |
| typeid/typeid-go/encoding_test.go | Swapped Must(Parse) calls for MustParse |
| typeid/typeid-go/bench_test.go | Swapped Must(Parse) for MustGenerate in benchmarks |
Comments suppressed due to low confidence (1)
typeid/typeid-go/bench_test.go:55
- The benchmark references
typeid.MustGenerate, but no such helper is defined. You’ll need to implementMustGenerate(similar toMustParse) or revert to usingGeneratewith error handling.
ids = append(ids, typeid.MustGenerate(prefix))
| if td.expectError { | ||
| assert.Error(t, err) | ||
| assert.Contains(t, err.Error(), "empty string is invalid TypeID") | ||
| assert.Contains(t, err.Error(), "cannot scan empty string into TypeID") |
There was a problem hiding this comment.
The test now expects "cannot scan empty string into TypeID", but TypeID.Scan still returns "empty string is invalid TypeID". Align the test or change the error message in TypeID.Scan so they match.
gcurtis
approved these changes
Jul 8, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Replace NullableID with sql.Null[TypeID] for nullable columns
Remove Must function from typeid.go to keep API surface small. Introduce MustParse as a helper only in tests (not part of public API)
How was it tested?
Updated unit tests and ran them.
Community Contribution License
All community contributions in this pull request are licensed to the project maintainers under the terms of the Apache 2 License.
By creating this pull request I represent that I have the right to license the contributions to the project maintainers under the Apache 2 License as stated in the Community Contribution License.