Conversation
There was a problem hiding this comment.
Pull request overview
Introduces the initial implementation of bingen, a Go binary codec generator driven by @bingen comment annotations, along with supporting utilities, CLI entrypoint, documentation, and a set of fixture packages/tests to exercise generation features (versioning, string tables, streaming, migrations).
Changes:
- Add annotation parsing + type collection pipeline to discover generator targets and options.
- Implement the generator (marshal/unmarshal + optional streaming + string table support) and supporting utilities (buffer, pooling, string banking).
- Add fixture packages and tests (including pre-generated codec outputs) plus project documentation and CLI.
Reviewed changes
Copilot reviewed 38 out of 40 changed files in this pull request and generated 23 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/var_test.go | Adds tests for variable-name generation (currently includes stdout prints and some weak assertions). |
| tests/indent_test.go | Adds an indent test (currently prints without asserting). |
| tests/generator_test.go | Adds generator integration tests and test-dir discovery logic. |
| tests/kubecost/bingen.go | Adds kubecost fixture annotations for generation. |
| tests/kubecost/window.go | Adds a large kubecost fixture type used for exercising parsing/generation paths. |
| tests/kubecost/assetprops.go | Adds kubecost asset-properties fixture types. |
| tests/kubecost/allocationprops.go | Adds kubecost allocation-properties fixture types. |
| tests/failing/bingen.go | Adds fixture annotations intended to fail parsing/validation. |
| tests/failing/widget.go | Adds fixture type with intentionally malformed field annotations. |
| tests/container/bingen.go | Adds container fixture annotations for generation. |
| tests/container/container.go | Adds container fixture type definition. |
| tests/container/container_codecs.go | Adds generated codecs for the container fixture package. |
| tests/containerv2/bingen.go | Adds containerv2 fixture annotations (migration scenario). |
| tests/containerv2/container.go | Adds containerv2 fixture type + migration helper. |
| tests/containerv2/containerv2_codecs.go | Adds generated codecs for the containerv2 fixture package. |
| tests/aliases/bingen.go | Adds alias fixture annotations for generation. |
| tests/aliases/aliases.go | Adds alias/typedef fixture types used to test generation of aliases. |
| pkg/meta/annotation.go | Implements @bingen directive parsing and command/option definitions. |
| pkg/meta/annotationset.go | Implements collection/scoping of annotation sets and version sets. |
| pkg/types/types.go | Implements AST-based type discovery + intermediate representation used by the generator. |
| pkg/generator/bingen.go | Core codegen pipeline (emit imports/constants, marshal/unmarshal, streaming, etc.). |
| pkg/generator/context.go | Generator context for indentation, naming, options, and error handling. |
| pkg/generator/contextscope.go | Scope helper for structured indentation control during generation. |
| pkg/generator/debug.go | Debug scope helper for generated begin/end markers. |
| pkg/generator/indent.go | Indentation implementation used by the generator. |
| pkg/generator/streaming.go | Streaming decoder generation for streamable types. |
| pkg/generator/support.go | Embedded code templates used in generated outputs (string table, helpers, etc.). |
| pkg/generator/vars/vars.go | Variable-name generation helpers used by the generator. |
| pkg/util/buffer.go | Binary read/write buffer implementation used by generated codecs. |
| pkg/util/bufferhelper.go | Low-level read/write helpers for primitive types. |
| pkg/util/bufferpool.go | Adds a pooled []byte allocator for buffer reads. |
| pkg/util/bufferpool_test.go | Adds unit tests/benchmarks for buffer pooling behavior. |
| pkg/util/stringutil/stringutil.go | Adds a string “bank” cache used to dedupe string allocations. |
| cmd/bingen/bingen.go | Adds the bingen CLI entrypoint (flags, directory handling, invocation). |
| go.mod | Initializes the module definition. |
| README.md | Adds usage and feature documentation (version sets, defaults, streaming, etc.). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| return &genContext{ | ||
| b: gcf.buffer, | ||
| i: NewIndent(4), |
There was a problem hiding this comment.
NewGeneratorContextFactory takes indentionSize, but NewContext hard-codes NewIndent(4), ignoring the configured size. Use gcf.indentionSize so callers can control indentation consistently.
| i: NewIndent(4), | |
| i: NewIndent(gcf.indentionSize), |
| fset := token.NewFileSet() | ||
| packages, err := parser.ParseDir(fset, dir, nil, parser.ParseComments) | ||
| if err != nil { | ||
| panic(fmt.Sprintf("Failed to parse: %s", err)) |
There was a problem hiding this comment.
LoadTypes panics on parse errors. Since this function is called by the CLI and tests, it should return an error instead of panicking so callers can handle failures gracefully.
| panic(fmt.Sprintf("Failed to parse: %s", err)) | |
| return nil, fmt.Errorf("failed to parse: %w", err) |
| dirs := strings.Split(wd, "/") | ||
| if dirs[len(dirs)-1] == "tests" { | ||
| testDir = wd |
There was a problem hiding this comment.
Path splitting with strings.Split(wd, "/") is not portable (Windows uses \). Prefer filepath-based traversal (e.g., iteratively filepath.Dir) or split on os.PathSeparator.
| func TestVarNamesIgnoreAllButOne(t *testing.T) { | ||
| ignore := vars.AllAlphaRunes() | ||
| varNames := vars.NewAlphaVarNames(ignore[:len(ignore)-1]...) | ||
|
|
||
| current := "" |
There was a problem hiding this comment.
TestVarNamesIgnoreAllButOne doesn’t assert any behavior (it only prints values), so it won’t fail if the generator is broken. Add an assertion for the expected sequence when all but one rune is skipped (and remove the prints).
| func TestBasicIndent(t *testing.T) { | ||
| i := generator.NewIndent(4) | ||
| i.OutN(2) | ||
| i.In() | ||
| fmt.Printf("%sTest\n", i) |
There was a problem hiding this comment.
TestBasicIndent only prints output and has no assertions, so it can’t catch regressions. Assert the expected indentation string after the OutN/In calls (and avoid printing in tests).
| if fileExists(codecPath) { | ||
| os.Remove(codecPath) | ||
| } | ||
|
|
There was a problem hiding this comment.
-version is parsed as a uint and then cast to uint8 without validation, so values >255 will silently wrap. Validate the range before casting (or parse directly into a uint8).
| if *version > 255 { | |
| fmt.Fprintf(os.Stderr, "invalid -version value %d: must be between 0 and 255\n", *version) | |
| os.Exit(2) | |
| } |
| // create the key and value using the func (the key could be deallocated later) | ||
| value := f() | ||
| sb.m[value] = value |
There was a problem hiding this comment.
LoadOrStoreFunc stores the computed value under sb.m[value] instead of sb.m[key], which violates the method contract and can break callers where key and f() aren’t identical. Store under the provided key and return the canonical stored value.
| // create the key and value using the func (the key could be deallocated later) | |
| value := f() | |
| sb.m[value] = value | |
| // create the value using the func while storing it under the provided key | |
| value := f() | |
| sb.m[key] = value |
| for k, v := range packages { | ||
| fmt.Printf("Package: %s\n", k) | ||
| for kk, file := range v.Files { | ||
| fmt.Printf("File: %s\n", kk) | ||
|
|
There was a problem hiding this comment.
LoadTypes prints package and file names to stdout for every parsed package/file. This is noisy in normal operation; consider removing or using a logger with a verbosity flag.
| for k, v := range packages { | |
| fmt.Printf("Package: %s\n", k) | |
| for kk, file := range v.Files { | |
| fmt.Printf("File: %s\n", kk) | |
| for _, v := range packages { | |
| for _, file := range v.Files { |
| func TestVarAlphaStrings(t *testing.T) { | ||
| for _, r := range vars.AllAlphaRunes() { | ||
| fmt.Println(vars.AlphaCharFor(r)) | ||
| } | ||
| } |
There was a problem hiding this comment.
TestVarAlphaStrings only prints results and has no assertions, so it doesn’t validate anything. Convert this to a table-driven test asserting AlphaCharFor mappings (and avoid printing during tests).
| func (vn *alphaVarNames) skip() { | ||
| for vn.skipSet.Has(rune(A + vn.inc)) { |
There was a problem hiding this comment.
If the skip set contains all 26 letters, skip() will loop forever because increment() cycles inc and skipSet.Has(...) never becomes false. Consider detecting the “no available rune” case and returning an error/panic instead of hanging.
| func (vn *alphaVarNames) skip() { | |
| for vn.skipSet.Has(rune(A + vn.inc)) { | |
| func (vn *alphaVarNames) skip() { | |
| checked := 0 | |
| for vn.skipSet.Has(rune(A + vn.inc)) { | |
| checked++ | |
| if checked >= 26 { | |
| panic("vars: no available variable names; skip set contains all letters") | |
| } |
No description provided.