TNTP-3729: generator for builtin go types #3
Conversation
bigbes
force-pushed
the
bigbes/TNTP-3729-generator-for-builtin-go-types
branch
from
d1b999f to
25d23bb
Compare
Pull Request Test Coverage Report for Build 16990606909Details
💛 - Coveralls |
bigbes
force-pushed
the
bigbes/TNTP-3729-generator-for-builtin-go-types
branch
4 times, most recently
from
a0c2c38 to
b3efea9
Compare
* supported optional types are int*, uint*, float*, bytes, string, bool * fully redo configuration for golangci-lint * simple tests for encoding/decoding roundtrip Closes TNTP-3729.
bigbes
force-pushed
the
bigbes/TNTP-3729-generator-for-builtin-go-types
branch
from
b3efea9 to
9db06ac
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR implements a code generator for optional builtin Go types including int*, uint*, float*, bytes, string, and bool. The generator creates consistent optional type wrappers with msgpack encoding/decoding support.
- Introduces a command-line code generator that produces optional type implementations from templates
- Generates optional types for all numeric types, string, bytes, and bool with consistent API methods
- Adds comprehensive test coverage for encoding/decoding roundtrips of all generated types
Reviewed Changes
Copilot reviewed 27 out of 28 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/generator/generator.go | Code generator implementation with template-based type generation |
| *.go (generated files) | Generated optional type implementations for builtin Go types |
| *_test.go | Test files covering encoding/decoding roundtrips for each type |
| helpers.go | Helper functions for msgpack encoding/decoding operations |
| errors.go | Custom error type for decode failures |
| .golangci.yml | Updated linter configuration with comprehensive rule set |
| go.mod | Added dependencies for msgpack, testing, and text processing |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| } | ||
|
|
||
| func (o Uint8) MustGet() uint8 { | ||
| if o.exists { |
There was a problem hiding this comment.
The logic in MustGet() is inverted. It should panic when the value does NOT exist (!o.exists), not when it exists.
| if o.exists { | |
| if !o.exists { |
Copilot uses AI. Check for mistakes.
| } | ||
|
|
||
| func (o Uint64) MustGet() uint64 { | ||
| if o.exists { |
There was a problem hiding this comment.
The logic in MustGet() is inverted. It should panic when the value does NOT exist (!o.exists), not when it exists.
| if o.exists { | |
| if !o.exists { |
Copilot uses AI. Check for mistakes.
| } | ||
|
|
||
| func (o Uint32) MustGet() uint32 { | ||
| if o.exists { |
There was a problem hiding this comment.
The logic in MustGet() is inverted. It should panic when the value does NOT exist (!o.exists), not when it exists.
| if o.exists { | |
| if !o.exists { |
Copilot uses AI. Check for mistakes.
| } | ||
|
|
||
| func (o Uint16) MustGet() uint16 { | ||
| if o.exists { |
There was a problem hiding this comment.
The logic in MustGet() is inverted. It should panic when the value does NOT exist (!o.exists), not when it exists.
| if o.exists { | |
| if !o.exists { |
Copilot uses AI. Check for mistakes.
| } | ||
|
|
||
| func (o Uint) MustGet() uint { | ||
| if o.exists { |
There was a problem hiding this comment.
The logic in MustGet() is inverted. It should panic when the value does NOT exist (!o.exists), not when it exists.
| if o.exists { | |
| if !o.exists { |
Copilot uses AI. Check for mistakes.
| } | ||
|
|
||
| func (o Float64) MustGet() float64 { | ||
| if o.exists { |
There was a problem hiding this comment.
The logic in MustGet() is inverted. It should panic when the value does NOT exist (!o.exists), not when it exists.
| if o.exists { | |
| if !o.exists { |
Copilot uses AI. Check for mistakes.
| } | ||
|
|
||
| func (o Float32) MustGet() float32 { | ||
| if o.exists { |
There was a problem hiding this comment.
The logic in MustGet() is inverted. It should panic when the value does NOT exist (!o.exists), not when it exists.
| if o.exists { | |
| if !o.exists { |
Copilot uses AI. Check for mistakes.
| } | ||
|
|
||
| func (o Bytes) MustGet() []byte { | ||
| if o.exists { |
There was a problem hiding this comment.
The logic in MustGet() is inverted. It should panic when the value does NOT exist (!o.exists), not when it exists.
| if o.exists { | |
| if !o.exists { |
Copilot uses AI. Check for mistakes.
| } | ||
|
|
||
| func (o Bool) MustGet() bool { | ||
| if o.exists { |
There was a problem hiding this comment.
The logic in MustGet() is inverted. It should panic when the value does NOT exist (!o.exists), not when it exists.
| if o.exists { | |
| if !o.exists { |
Copilot uses AI. Check for mistakes.
| } | ||
|
|
||
| func (o {{.Name}}) MustGet() {{.Type}} { | ||
| if o.exists { |
There was a problem hiding this comment.
The logic in the MustGet() template is inverted. It should panic when the value does NOT exist (!o.exists), not when it exists.
| if o.exists { | |
| if !o.exists { |
Copilot uses AI. Check for mistakes.
| disable: | ||
| - return | ||
| - assign-exclusive | ||
| - assign-expr |
There was a problem hiding this comment.
Please, add comments: why do we disable the checks? After a while, it's not clear.
| disable: | ||
| - dupl | ||
| - cyclop | ||
| - wsl | ||
| - nlreturn | ||
| - ireturn | ||
| - wrapcheck |
There was a problem hiding this comment.
Please, add comments: why do we disable the checks? After a while, it's not clear.
| exclusions: | ||
| rules: | ||
| - path-except: cmd/generator/*.go | ||
| linters: | ||
| - forbidigo | ||
| - gochecknoglobals |
| @@ -10,6 +10,10 @@ Versioning](http://semver.org/spec/v2.0.0.html) except to the first release. | |||
|
|
|||
| ### Added | |||
|
|
|||
| * Implemented optional types for builtin go types int*, uint*, float*, | |||
There was a problem hiding this comment.
See:
https://keepachangelog.com/en/1.0.0/
| * Implemented optional types for builtin go types int*, uint*, float*, | |
| - Implemented optional types for builtin go types int*, uint*, float*, |
|
|
||
| type Bool struct { | ||
| value bool | ||
| exists bool | ||
| } | ||
|
|
||
| func SomeBool(value bool) Bool { | ||
| return Bool{ | ||
| value: value, | ||
| exists: true, | ||
| } | ||
| } | ||
|
|
||
| func NoneBool() Bool { | ||
| return Bool{ | ||
| exists: false, | ||
| value: zero[bool](), | ||
| } | ||
| } | ||
|
|
||
| func (o Bool) IsSome() bool { | ||
| return o.exists | ||
| } |
There was a problem hiding this comment.
Please add comments to the public API methods and types. Despite the fact that it will be generated code and comments generated, but this is a general rule in our repositories.
| type Bool struct { | |
| value bool | |
| exists bool | |
| } | |
| func SomeBool(value bool) Bool { | |
| return Bool{ | |
| value: value, | |
| exists: true, | |
| } | |
| } | |
| func NoneBool() Bool { | |
| return Bool{ | |
| exists: false, | |
| value: zero[bool](), | |
| } | |
| } | |
| func (o Bool) IsSome() bool { | |
| return o.exists | |
| } | |
| // Bool is ... | |
| type Bool struct { | |
| value bool | |
| exists bool | |
| } | |
| // SomeBool returns... | |
| func SomeBool(value bool) Bool { | |
| return Bool{ | |
| value: value, | |
| exists: true, | |
| } | |
| } | |
| // NoneBool returns ... | |
| func NoneBool() Bool { | |
| return Bool{ | |
| exists: false, | |
| value: zero[bool](), | |
| } | |
| } | |
| // IsSome returns ... | |
| func (o Bool) IsSome() bool { | |
| return o.exists | |
| } |
|
|
||
| err = generateAndWrite() | ||
| if err != nil { | ||
| fmt.Println("failed to generate or write code: ", err.Error()) |
There was a problem hiding this comment.
It should work fine.
| fmt.Println("failed to generate or write code: ", err.Error()) | |
| fmt.Println("failed to generate or write code: ", err) |
| // Check if output directory exists and is directory. | ||
| switch fInfo, err := os.Stat(absOutputDirectory); { | ||
| case err != nil: | ||
| fmt.Println("failed to stat output directory: ", err.Error()) |
There was a problem hiding this comment.
etc
| fmt.Println("failed to stat output directory: ", err.Error()) | |
| fmt.Println("failed to stat output directory: ", err) |
| // Get absolute path for output directory. | ||
| absOutputDirectory, err := filepath.Abs(outputDirectory) | ||
| if err != nil { | ||
| fmt.Printf("failed to get absolute path for output directory (%s): %s\n", outputDirectory, err.Error()) |
There was a problem hiding this comment.
etc
| fmt.Printf("failed to get absolute path for output directory (%s): %s\n", outputDirectory, err.Error()) | |
| fmt.Printf("failed to get absolute path for output directory (%s): %s\n", outputDirectory, err) |
| func decodeInt(decoder *msgpack.Decoder) (int, error) { | ||
| return decoder.DecodeInt() | ||
| } | ||
|
|
||
| func decodeInt8(decoder *msgpack.Decoder) (int8, error) { | ||
| return decoder.DecodeInt8() | ||
| } | ||
|
|
||
| func decodeInt16(decoder *msgpack.Decoder) (int16, error) { | ||
| return decoder.DecodeInt16() | ||
| } |
There was a problem hiding this comment.
Please, add a comment: why do we need such helper functions? It is not obvious. Probably the comment could be placed into the header of the file.
There was a problem hiding this comment.
'helpers, utils` etc usually a bad filename. As a result, unrelated functions accumulate in these files. It is a good idea to choose more specific name and split the file into several ones.
It is not critical here, so up to you.
No documentation, yet, will be added in separate PR.
Closes TNTP-3729.