Skip to content

Add descriptor-aware PROTO/ENUM formatting plugins#151

Merged
apstndb merged 5 commits into
mainfrom
issue-149-protofmt
Jun 3, 2026
Merged

Add descriptor-aware PROTO/ENUM formatting plugins#151
apstndb merged 5 commits into
mainfrom
issue-149-protofmt

Conversation

@apstndb

@apstndb apstndb commented Jun 3, 2026

Copy link
Copy Markdown
Owner

Summary

  • Add a new protofmt package with opt-in descriptor-aware PROTO and ENUM formatting plugins.
  • Support dynamic descriptor sets, generated/registered protobuf types, resolver composition, native protobuf marshal/unmarshal options, typed NULL handling, and descriptor-free fallthrough.
  • Keep descriptor loading/compilation in applications, keep the root package and preset defaults descriptor-free, and document display caveats such as multiline prototext in nested or delimited cells.
  • Use a smaller enum-only resolver interface for ENUM formatting while full ProtoEnumResolver support remains available for PROTO, extension lookup, Any expansion, and resolver composition.
  • Add coverage for dynamic messages, protoregistry.GlobalTypes, resolver semantics, malformed wire payloads, enum fallback, protobuf option resolver wiring, and nested ARRAY/STRUCT dispatch.

Fixes #149.

Testing

  • go test ./protofmt
  • go test ./...
  • make check

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces the protofmt package, which provides opt-in, descriptor-aware formatting plugins for Spanner PROTO and ENUM values. Feedback focuses on improving GoDoc link resolution on pkg.go.dev by using fully qualified paths and receiver types, as well as optimizing lookup performance on the hot path by filtering out nil resolvers during construction in ComposeProtoEnumResolvers to avoid reflection overhead.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread protofmt/doc.go Outdated
Comment thread protofmt/protofmt.go Outdated
Comment thread protofmt/protofmt.go Outdated
Comment thread protofmt/protofmt.go
Comment thread protofmt/protofmt.go

@apstndb apstndb left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed against the issue #149 acceptance criteria, with the branch checked out locally (go test ./protofmt/, make lint both pass).

This is a clean, well-scoped implementation. Highlights:

  • protofmt subpackage keeps dynamicpb/protodesc out of the root import graph for users who don't need descriptor-aware display.
  • Resolver-first API; ProtoEnumResolver is satisfied by both *dynamicpb.Types and *protoregistry.Types, and is directly assignable to proto.UnmarshalOptions.Resolver / prototext.MarshalOptions.Resolver (verified by compile-time var _ assertions).
  • The prototext-instability concern is handled correctly: docs say output is not byte-stable, and the dynamic-message test uses unmarshal + proto.Equal round-trip rather than string comparison.
  • ComposeProtoEnumResolvers semantics (exact-NotFound to continue, wrapped errors surfaced, exact NotFound when all miss) match the spec and are well covered.
  • Defensive details are good: int32 enum-range guard, typed-nil resolver detection, stringWire value-kind validation.

I separately verified that dynamicpb.Types and protoregistry.GlobalTypes both return the exact protoregistry.NotFound sentinel (not wrapped) for all five lookup methods, so the == comparison in isExactNotFound is correct in practice.

A few minor/optional comments inline; none are blocking.

Comment thread protofmt/protofmt.go
Comment thread protofmt/protofmt_test.go
Comment thread protofmt/doc.go
Comment thread protofmt/protofmt.go

@apstndb apstndb left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the PR against issue #149 and the existing review threads. The implementation is generally clean and go test ./protofmt, make lint, and make check pass locally. I left two non-blocking inline comments: one coverage gap around proving resolver wiring through Any/extensions, and one API-surface question for enum-only custom resolvers.

Comment thread protofmt/protofmt_test.go
Comment thread protofmt/protofmt.go Outdated
@apstndb

apstndb commented Jun 3, 2026

Copy link
Copy Markdown
Owner Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces the protofmt package, which provides descriptor-aware PROTO and ENUM formatting plugins for spanvalue format configs, along with corresponding documentation and tests. The review feedback focuses on performance optimizations: pre-configuring marshal/unmarshal options and performing reflection-based nil resolver checks at construction time rather than repeatedly during cell formatting in FormatProtoTextValue and FormatEnumNameValue, and returning the single active resolver directly in ComposeProtoEnumResolvers to avoid delegation overhead.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread protofmt/protofmt.go
Comment thread protofmt/protofmt.go
Comment thread protofmt/protofmt.go
@apstndb

apstndb commented Jun 3, 2026

Copy link
Copy Markdown
Owner Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces the protofmt package, which provides descriptor-aware PROTO and ENUM formatting plugins for spanvalue format configurations, along with comprehensive unit tests. The reviewer feedback highlights opportunities to improve robustness by adding defensive nil checks for resolved message and enum types to prevent panics from buggy resolvers, and suggests an optimization in ComposeProtoEnumResolvers when no active resolvers are provided.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread protofmt/protofmt.go
Comment thread protofmt/protofmt.go
Comment thread protofmt/protofmt.go
@apstndb

apstndb commented Jun 3, 2026

Copy link
Copy Markdown
Owner Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new package protofmt which provides opt-in, descriptor-aware PROTO and ENUM formatting plugins for spanvalue format configurations. It includes interfaces and implementations for resolving protobuf message and enum types, formatting functions (FormatProtoTextValue and FormatEnumNameValue), helper functions to compose resolvers, and comprehensive unit tests. Documentation and README files have been updated accordingly. There are no review comments, and I have no feedback to provide.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

@apstndb apstndb marked this pull request as ready for review June 3, 2026 22:15
@apstndb

apstndb commented Jun 3, 2026

Copy link
Copy Markdown
Owner Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces the protofmt package, which provides opt-in, descriptor-aware PROTO and ENUM formatting plugins for spanvalue format configurations. It includes implementation files, comprehensive unit tests, and updates to the project's documentation (README.md and doc.go) to describe the new package and its usage. I have no feedback to provide as there are no review comments to assess.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

@apstndb apstndb left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review of the updated branch (HEAD c22c151). Checked out locally; make lint -> 0 issues, go test ./protofmt/ -> ok.

All four of my earlier comments are resolved, and the follow-up performance/hardening pass is solid:

  • Resolver wiring proven, not just asserted. TestFormatProtoTextValue_usesMarshalResolverForAnyExpansion now demonstrates marshal.Resolver actually drives google.protobuf.Any expansion via a dynamic Envelope{ Any } descriptor. Nice — this is much stronger than the original round-trip-only coverage.
  • GlobalTypes missing-type path now has explicit ErrFallthrough cases for both PROTO and ENUM, closing the coverage gap I flagged. I independently re-confirmed dynamicpb.Types and protoregistry.GlobalTypes return the exact protoregistry.NotFound sentinel for all five lookups, so isExactNotFound's == remains correct.
  • Hot-path refactor is clean. Hoisting isNilResolver + unmarshal/marshal.Resolver setup to construction time (two-closure split) removes per-value reflection and option copies without changing behavior. The messageType == nil / enumType == nil guards plus their tests are good defense against contract-violating custom resolvers.
  • Compose filtering nil resolvers at construction, returning the single active resolver directly, and the empty-compositeResolver NotFound fallback are all well covered.
  • EnumResolver segregation, doc GlobalTypes/blank-import note, and the README multiline-newline caveat all landed.

No blocking issues. One optional follow-up inline; otherwise this LGTM from my side. (Gemini's two most recent reviews also report no remaining feedback.)

Nit (optional, not worth a separate change on its own): the typed-NULL guard preamble (code != X -> fallthrough, IsNull -> GetNullString) is now duplicated across the nil-resolver and active closures in both plugins (4 copies). If it ever grows, a small shared guard helper would prevent drift.

Comment thread protofmt/protofmt.go
@apstndb apstndb merged commit c7c9053 into main Jun 3, 2026
2 checks passed
@apstndb apstndb deleted the issue-149-protofmt branch June 3, 2026 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add descriptor-aware PROTO/ENUM formatting plugins

1 participant