Skip to content

fix(localizer): remove redundant PBFieldOpts concat in protogen error template#423

Merged
wenchy merged 1 commit into
masterfrom
rm-pb-field-opts
Jun 30, 2026
Merged

fix(localizer): remove redundant PBFieldOpts concat in protogen error template#423
wenchy merged 1 commit into
masterfrom
rm-pb-field-opts

Conversation

@Kybxd

@Kybxd Kybxd commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

Problem

When protogen reports a field error, the rendered message duplicates the prop segment:

Workbook: Item.xlsx (alias: Item)
Worksheet: #UnionItem
NameCellPos: D9
NameCell: ItemID
TypeCellPos: D9
TypeCell: int64|{"refer:ItemConf.ID"}|"refer:ItemConf.ID"
                                          ^^^^^^^^^^^^^^^^^^^^^ duplicated
Reason: failed to parse field prop: ...

The protogen template renders TypeCell as:

TypeCell: {{.TypeCell}}{{ if .PBFieldOpts }}|{{.PBFieldOpts}}{{ end }}

Root cause

Audited every write site of xerrors.KeyTypeCell and xerrors.KeyPBFieldOpts:

Write site Content Contains prop?
protogen/util.go wrapDebugErrheader.getTypeCell raw type-row line ✅ full int64|{...}
importer/book/node.go Node.DebugKVn.Value raw YAML/JSON scalar ✅ full int64|{...}
~25 sites writing KeyPBFieldOpts prop.Text / desc.Prop.Text / field.opts a substring of the above

So in the protogen path, TypeCell always already carries the prop, and PBFieldOpts is just the same substring appended again.

confgen and default templates do not reference PBFieldOpts at all, so removing the concat has no impact there.

Fix

Drop the |{{.PBFieldOpts}} suffix from TypeCell in both zh.yaml and en.yaml.

- 类型单元格数据: {{.TypeCell}}{{ if .PBFieldOpts }}|{{.PBFieldOpts}}{{ end }}
+ 类型单元格数据: {{.TypeCell}}

- TypeCell: {{.TypeCell}}{{ if .PBFieldOpts }}|{{.PBFieldOpts}}{{ end }}
+ TypeCell: {{.TypeCell}}

Why xerrors.KeyPBFieldOpts is kept

  • It still appears as its own line in the debug field block (xerrors/desc.go fieldsString), useful for locating prop parsing failures — note that prop.Text has its outer {} stripped, so it isn't strictly identical to TypeCell.
  • Removing the constant would force changes across ~25 call sites with no functional benefit.

After

TypeCell: int64|{"refer:ItemConf.ID"}
TypeCell: failed to parse field prop: ...

Verification

  • go test ./internal/localizer/... ./internal/x/xerrors/...
  • go test ./internal/protogen/... ./internal/confgen/...

Scope

Localized error template only; no production code changes.

… template

The protogen error template appended PBFieldOpts after TypeCell as
"{{.TypeCell}}|{{.PBFieldOpts}}", but every protogen write site of
KeyTypeCell already stores the full type-cell string (including its
`|{prop}` segment):

- protogen/util.go (wrapDebugErr) writes header.getTypeCell(cursor),
  which is the raw type-row line untouched by prop stripping.
- importer/book/node.go (Node.DebugKV) writes n.Value, the raw scalar
  from the YAML/JSON document, also including the full prop.

Meanwhile KeyPBFieldOpts is written from prop.Text / desc.Prop.Text,
which is a substring of the same type cell. The template therefore
duplicated the prop section on every protogen error, e.g.:

  TypeCell: int64|{"refer:ItemConf.ID"}|"refer:ItemConf.ID"

confgen and default templates do not reference PBFieldOpts at all, so
no path actually needs the concatenation.

Drop the `|{{.PBFieldOpts}}` suffix from both zh.yaml and en.yaml.
KeyPBFieldOpts is kept since it still shows up as its own line in the
debug field block (fieldsString) and is useful for locating prop
parsing failures (prop.Text has braces stripped, so it is not strictly
equal to TypeCell).
@github-actions

Copy link
Copy Markdown

The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedJun 30, 2026, 6:30 AM

@Kybxd Kybxd requested a review from wenchy June 30, 2026 06:33
@codecov

codecov Bot commented Jun 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.95%. Comparing base (ba5466e) to head (c330c1a).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #423   +/-   ##
=======================================
  Coverage   74.95%   74.95%           
=======================================
  Files          88       88           
  Lines        9408     9408           
=======================================
  Hits         7052     7052           
  Misses       1784     1784           
  Partials      572      572           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@wenchy wenchy merged commit 9c744b8 into master Jun 30, 2026
9 checks passed
@wenchy wenchy deleted the rm-pb-field-opts branch June 30, 2026 07:14
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.

2 participants