From 2bacdd2eca249254c696d269b1a2471e12ae2e99 Mon Sep 17 00:00:00 2001 From: Kybxd <627940450@qq.com> Date: Tue, 24 Mar 2026 16:49:14 +0800 Subject: [PATCH 1/3] fix(protogen): add correct cell positioners for struct and union type sheet parsing --- internal/protogen/sheet_mode.go | 52 +++- internal/protogen/sheet_mode_test.go | 366 +++++++++++++++++++++++++++ 2 files changed, 417 insertions(+), 1 deletion(-) create mode 100644 internal/protogen/sheet_mode_test.go diff --git a/internal/protogen/sheet_mode.go b/internal/protogen/sheet_mode.go index 390ad01f..570f24a5 100644 --- a/internal/protogen/sheet_mode.go +++ b/internal/protogen/sheet_mode.go @@ -115,17 +115,36 @@ func extractStructTypeInfo(sheet *book.Sheet, typeName, parentFilename string, p return nil } +// verticalPositioner correctly maps positions for LAYOUT_VERTICAL sheets +// (e.g., struct type sheets) where cursor iterates over data rows +// instead of columns. +type verticalPositioner struct { + tabler book.Tabler + dataRow int // 0-based data start row in tabler's coordinate +} + +func (p *verticalPositioner) Position(row, col int) string { + // row: virtual header row index (e.g., 0 for Name col, 1 for Type col) + // col: cursor (field index), maps to actual data row + return p.tabler.Position(p.dataRow+col, row) +} + func parseStructType(ws *internalpb.Worksheet, sheet *book.Sheet, parser book.SheetParser, gen *Generator, debugBookName, debugSheetName string) error { desc := &internalpb.StructDescriptor{} if err := parser.Parse(desc, sheet); err != nil { return err } bp := newTableParser("struct", "", "", gen) + t := sheet.Tabler() shHeader := &tableHeader{ Header: &tableparser.Header{ NameRow: 1, TypeRow: 2, }, + Positioner: &verticalPositioner{ + tabler: t, + dataRow: t.BeginRow() + 1, // StructDescriptor's datarow is 2 (1-based) + }, } for _, field := range desc.Fields { shHeader.nameRowData = append(shHeader.nameRowData, strings.TrimSpace(field.Name)) @@ -205,6 +224,31 @@ func extractUnionTypeInfo(sheet *book.Sheet, typeName, parentFilename string, pa return nil } +// unionFieldPositioner correctly maps positions for union type sheets where +// cursor iterates over field columns within a specific union value row. +type unionFieldPositioner struct { + tabler book.Tabler + valueRow int // 0-based row of current union value in tabler's coordinate + fieldStartCol int // 0-based column where Field1 starts +} + +func (p *unionFieldPositioner) Position(row, col int) string { + // row param is unused since name/type/note are all in the same cell (different lines). + // col is the cursor (field index within this value), maps to actual column. + return p.tabler.Position(p.valueRow, p.fieldStartCol+col) +} + +// findFieldStartCol finds the 0-based column index of "Field1" in the header row. +func findFieldStartCol(t book.Tabler) int { + headerRow := t.GetRow(t.BeginRow()) // namerow=1, 0-based: BeginRow+0 + for i, cell := range headerRow { + if cell == colFieldPrefix+"1" { + return i + } + } + return 0 // fallback +} + func parseUnionType(ws *internalpb.Worksheet, sheet *book.Sheet, parser book.SheetParser, gen *Generator, debugBookName, debugSheetName string) error { desc := &internalpb.UnionDescriptor{} if err := parser.Parse(desc, sheet); err != nil { @@ -232,7 +276,8 @@ func parseUnionType(ws *internalpb.Worksheet, sheet *book.Sheet, parser book.She // create a book parser bp := newTableParser("union", "", "", gen) - + t := sheet.Tabler() + fieldStartCol := findFieldStartCol(t) shHeader := &tableHeader{ Header: &tableparser.Header{ NameRow: 1, @@ -241,6 +286,11 @@ func parseUnionType(ws *internalpb.Worksheet, sheet *book.Sheet, parser book.She TypeLine: 2, NoteLine: 3, }, + Positioner: &unionFieldPositioner{ + tabler: t, + valueRow: t.BeginRow() + 1 + i, // datarow=2 (1-based), i is the value index + fieldStartCol: fieldStartCol, + }, nameRowData: value.Fields, typeRowData: value.Fields, noteRowData: value.Fields, diff --git a/internal/protogen/sheet_mode_test.go b/internal/protogen/sheet_mode_test.go new file mode 100644 index 00000000..eb2a37e8 --- /dev/null +++ b/internal/protogen/sheet_mode_test.go @@ -0,0 +1,366 @@ +package protogen + +import ( + "testing" + + "github.com/tableauio/tableau/internal/importer/book" +) + +func TestVerticalPositioner_Position(t *testing.T) { + // Simulate a non-transposed struct type sheet: + // A(col0) B(col1) + // R1: Name Type <- header (namerow=1) + // R2: Field0 int32 <- data row 0 (datarow=2) + // R3: Field1 string <- data row 1 + // R4: Field2 bool <- data row 2 + // ... + // R6: Field4 float <- data row 4 + table := book.NewTable([][]string{ + {"Name", "Type"}, // row 0 (header) + {"Field0", "int32"}, // row 1 (data row 0) + {"Field1", "string"}, // row 2 (data row 1) + {"Field2", "bool"}, // row 3 (data row 2) + {"Field3", "int64"}, // row 4 (data row 3) + {"Field4", "float"}, // row 5 (data row 4) + }) + + p := &verticalPositioner{ + tabler: table, + dataRow: table.BeginRow() + 1, // datarow=2 (1-based), 0-based: 1 + } + + tests := []struct { + name string + row int // virtual header row index: 0=Name col, 1=Type col + col int // cursor (field index) + expect string + }{ + { + name: "NameRow-cursor0", + row: 0, + col: 0, + expect: "A2", // dataRow(1)+cursor(0)=row1, col0=A => A2 + }, + { + name: "TypeRow-cursor0", + row: 1, + col: 0, + expect: "B2", // dataRow(1)+cursor(0)=row1, col1=B => B2 + }, + { + name: "NameRow-cursor4", + row: 0, + col: 4, + expect: "A6", // dataRow(1)+cursor(4)=row5, col0=A => A6 + }, + { + name: "TypeRow-cursor4", + row: 1, + col: 4, + expect: "B6", // dataRow(1)+cursor(4)=row5, col1=B => B6 + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := p.Position(tt.row, tt.col) + if got != tt.expect { + t.Errorf("verticalPositioner.Position(%d, %d) = %v, want %v", tt.row, tt.col, got, tt.expect) + } + }) + } +} + +func TestVerticalPositioner_Position_Transposed(t *testing.T) { + // Simulate a transposed struct type sheet: + // The underlying table is: + // A(col0) B(col1) C(col2) D(col3) E(col4) F(col5) + // R1: Name Field0 Field1 Field2 Field3 Field4 + // R2: Type int32 string bool int64 float + // + // After transposing, the virtual layout becomes: + // A(col0) B(col1) + // R1: Name Type + // R2: Field0 int32 + // R3: Field1 string + // ... + // R6: Field4 float + // + // But Position on TransposedTable swaps row/col back to original coordinates. + table := book.NewTable([][]string{ + {"Name", "Field0", "Field1", "Field2", "Field3", "Field4"}, + {"Type", "int32", "string", "bool", "int64", "float"}, + }) + transposed := table.Transpose() + + p := &verticalPositioner{ + tabler: transposed, + dataRow: transposed.BeginRow() + 1, // datarow=2 (1-based), 0-based in transposed: 1 + } + + tests := []struct { + name string + row int + col int + expect string + }{ + { + name: "NameRow-cursor0", + row: 0, + col: 0, + expect: "B1", // transposed.Position(1, 0) => table.Position(0, 1) => B1 + }, + { + name: "TypeRow-cursor0", + row: 1, + col: 0, + expect: "B2", // transposed.Position(1, 1) => table.Position(1, 1) => B2 + }, + { + name: "NameRow-cursor4", + row: 0, + col: 4, + expect: "F1", // transposed.Position(5, 0) => table.Position(0, 5) => F1 + }, + { + name: "TypeRow-cursor4", + row: 1, + col: 4, + expect: "F2", // transposed.Position(5, 1) => table.Position(1, 5) => F2 + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := p.Position(tt.row, tt.col) + if got != tt.expect { + t.Errorf("verticalPositioner(transposed).Position(%d, %d) = %v, want %v", tt.row, tt.col, got, tt.expect) + } + }) + } +} + +func TestUnionFieldPositioner_Position(t *testing.T) { + // Simulate a non-transposed union type sheet: + // A(col0) B(col1) C(col2) D(col3) E(col4) F(col5) + // R1: Name Alias Type Field1 Field2 Field3 <- header + // R2: Value0 Alias0 Type0 <- value 0 + // R3: Value1 Alias1 Type1 <- value 1 + // ... + // R228: Value226 Alias226 Type226 <- value 226 + table := book.NewTable(make([][]string, 228)) // 228 rows + + tests := []struct { + name string + valueRow int + fieldStartCol int + row int // unused in unionFieldPositioner + col int // cursor (field index) + expect string + }{ + { + name: "value0-field0", + valueRow: 1, // datarow=2 (1-based), BeginRow(0)+1+0=1 + fieldStartCol: 3, // Field1 at col D + row: 0, + col: 0, + expect: "D2", // Position(1, 3) => D2 + }, + { + name: "value0-field2", + valueRow: 1, + fieldStartCol: 3, + row: 0, + col: 2, + expect: "F2", // Position(1, 5) => F2 + }, + { + name: "value226-field0-D228", + valueRow: 227, // BeginRow(0)+1+226=227 + fieldStartCol: 3, + row: 0, + col: 0, + expect: "D228", // Position(227, 3) => D228 + }, + { + name: "value226-field1-E228", + valueRow: 227, + fieldStartCol: 3, + row: 0, + col: 1, + expect: "E228", // Position(227, 4) => E228 + }, + { + name: "row-param-ignored", + valueRow: 227, + fieldStartCol: 3, + row: 1, // different row param, should still produce same result + col: 0, + expect: "D228", // row param is ignored, Position(227, 3) => D228 + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + p := &unionFieldPositioner{ + tabler: table, + valueRow: tt.valueRow, + fieldStartCol: tt.fieldStartCol, + } + got := p.Position(tt.row, tt.col) + if got != tt.expect { + t.Errorf("unionFieldPositioner.Position(%d, %d) = %v, want %v", tt.row, tt.col, got, tt.expect) + } + }) + } +} + +func TestUnionFieldPositioner_Position_Transposed(t *testing.T) { + // Simulate a transposed union type sheet: + // The underlying table is: + // A(col0) B(col1) C(col2) ... + // R1: Name Value0 Value1 ... + // R2: Alias Alias0 Alias1 ... + // R3: Type Type0 Type1 ... + // R4: Field1 ... + // R5: Field2 ... + // + // After transposing, virtual layout: + // A B C D E + // R1: Name Alias Type Field1 Field2 + // R2: Value0 Alias0 Type0 + // R3: Value1 Alias1 Type1 + // + // TransposedTable.Position(row, col) => table.Position(col, row) + + table := book.NewTable([][]string{ + {"Name", "Value0", "Value1"}, + {"Alias", "Alias0", "Alias1"}, + {"Type", "Type0", "Type1"}, + {"Field1", "cell00", "cell10"}, + {"Field2", "cell01", "cell11"}, + }) + transposed := table.Transpose() + + tests := []struct { + name string + valueRow int + fieldStartCol int + row int + col int + expect string + }{ + { + name: "value0-field0", + valueRow: 1, // BeginRow(0)+1+0=1 + fieldStartCol: 3, // Field1 at virtual col 3 + row: 0, + col: 0, + // transposed.Position(1, 3) => table.Position(3, 1) => B4 + expect: "B4", + }, + { + name: "value0-field1", + valueRow: 1, + fieldStartCol: 3, + row: 0, + col: 1, + // transposed.Position(1, 4) => table.Position(4, 1) => B5 + expect: "B5", + }, + { + name: "value1-field0", + valueRow: 2, // BeginRow(0)+1+1=2 + fieldStartCol: 3, + row: 0, + col: 0, + // transposed.Position(2, 3) => table.Position(3, 2) => C4 + expect: "C4", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + p := &unionFieldPositioner{ + tabler: transposed, + valueRow: tt.valueRow, + fieldStartCol: tt.fieldStartCol, + } + got := p.Position(tt.row, tt.col) + if got != tt.expect { + t.Errorf("unionFieldPositioner(transposed).Position(%d, %d) = %v, want %v", tt.row, tt.col, got, tt.expect) + } + }) + } +} + +func TestFindFieldStartCol(t *testing.T) { + tests := []struct { + name string + tabler book.Tabler + expect int + }{ + { + name: "standard-union-header-with-type", + tabler: book.NewTable([][]string{ + {"Name", "Alias", "Type", "Field1", "Field2"}, + {"v1", "a1", "t1", "f1\nint32", "f2\nstring"}, + }), + expect: 3, // Field1 at col D (index 3) + }, + { + name: "union-header-without-number-and-type", + tabler: book.NewTable([][]string{ + {"Name", "Alias", "Field1", "Field2"}, + {"v1", "a1", "f1\nint32", "f2\nstring"}, + }), + expect: 2, // Field1 at col C (index 2) + }, + { + name: "union-header-with-number", + tabler: book.NewTable([][]string{ + {"Number", "Name", "Alias", "Type", "Field1", "Field2"}, + {"1", "v1", "a1", "t1", "f1\nint32", "f2\nstring"}, + }), + expect: 4, // Field1 at col E (index 4) + }, + { + name: "no-field-columns-fallback", + tabler: book.NewTable([][]string{ + {"Name", "Alias", "Type"}, + {"v1", "a1", "t1"}, + }), + expect: 0, // fallback to 0 + }, + { + name: "transposed-union-header", + tabler: book.NewTable([][]string{ + // Underlying table (before transpose): + // A B C + // R1: Name v1 v2 + // R2: Alias a1 a2 + // R3: Type t1 t2 + // R4: Field1 f1 f3 + // R5: Field2 f2 f4 + {"Name", "v1", "v2"}, + {"Alias", "a1", "a2"}, + {"Type", "t1", "t2"}, + {"Field1", "f1", "f3"}, + {"Field2", "f2", "f4"}, + }).Transpose(), + // Transposed virtual header row (GetRow(BeginRow=0)): + // GetRow(0) => table.getCol(0) => ["Name", "Alias", "Type", "Field1", "Field2"] + // Field1 at index 3 + expect: 3, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := findFieldStartCol(tt.tabler) + if got != tt.expect { + t.Errorf("findFieldStartCol() = %v, want %v", got, tt.expect) + } + }) + } +} From 96b0b15e9cfb2e026263d8369432e34ab50c4df8 Mon Sep 17 00:00:00 2001 From: Kybxd <627940450@qq.com> Date: Tue, 7 Apr 2026 15:40:08 +0800 Subject: [PATCH 2/3] refactor: Refactor positioners and add Note column support --- internal/protogen/sheet_mode.go | 81 ++++-- internal/protogen/sheet_mode_test.go | 376 ++++++++++++++++++--------- 2 files changed, 317 insertions(+), 140 deletions(-) diff --git a/internal/protogen/sheet_mode.go b/internal/protogen/sheet_mode.go index 570f24a5..36b1a7ed 100644 --- a/internal/protogen/sheet_mode.go +++ b/internal/protogen/sheet_mode.go @@ -2,7 +2,9 @@ package protogen import ( "fmt" + "strconv" "strings" + "sync" "github.com/tableauio/tableau/internal/importer/book" "github.com/tableauio/tableau/internal/importer/book/tableparser" @@ -19,6 +21,7 @@ const ( colNumber = "Number" // name of column "Number" colName = "Name" // name of column "Name" colType = "Type" // name of column "Type" + colNote = "Note" // name of column "Note" colAlias = "Alias" // name of column "Alias" colFieldPrefix = "Field" // name of column field prefix "Field" ) @@ -115,18 +118,56 @@ func extractStructTypeInfo(sheet *book.Sheet, typeName, parentFilename string, p return nil } +// basePositioner holds common fields for positioners that need to resolve +// column names to physical column indices. The colMap is lazily initialized +// on the first call to colIndex via sync.Once. +type basePositioner struct { + tabler book.Tabler + colMap map[string]int // column name -> 0-based physical column index + once sync.Once +} + +// colIndex returns the 0-based physical column index for the given column name. +// On the first call, it scans the header row to build the colMap. +// Returns (index, true) if found, or (0, false) if the column does not exist. +func (b *basePositioner) colIndex(name string) (int, bool) { + b.once.Do(func() { + headerRow := b.tabler.GetRow(b.tabler.BeginRow()) + b.colMap = make(map[string]int, len(headerRow)) + for i, cell := range headerRow { + if cell != "" { + b.colMap[cell] = i + } + } + }) + idx, ok := b.colMap[name] + return idx, ok +} + +// verticalRowNames maps virtual header row indices to column names. +// Index 0 corresponds to NameRow, 1 to TypeRow, 2 to NoteRow. +var verticalRowNames = []string{colName, colType, colNote} + // verticalPositioner correctly maps positions for LAYOUT_VERTICAL sheets // (e.g., struct type sheets) where cursor iterates over data rows // instead of columns. type verticalPositioner struct { - tabler book.Tabler + basePositioner dataRow int // 0-based data start row in tabler's coordinate } func (p *verticalPositioner) Position(row, col int) string { - // row: virtual header row index (e.g., 0 for Name col, 1 for Type col) + // row: virtual header row index (e.g., 0 for Name, 1 for Type, 2 for Note) // col: cursor (field index), maps to actual data row - return p.tabler.Position(p.dataRow+col, row) + if row < 0 || row >= len(verticalRowNames) { + return "" + } + colIdx, ok := p.colIndex(verticalRowNames[row]) + if !ok { + // The requested column (e.g., Note) does not exist in this table. + return "" + } + return p.tabler.Position(p.dataRow+col, colIdx) } func parseStructType(ws *internalpb.Worksheet, sheet *book.Sheet, parser book.SheetParser, gen *Generator, debugBookName, debugSheetName string) error { @@ -140,10 +181,11 @@ func parseStructType(ws *internalpb.Worksheet, sheet *book.Sheet, parser book.Sh Header: &tableparser.Header{ NameRow: 1, TypeRow: 2, + NoteRow: 3, }, Positioner: &verticalPositioner{ - tabler: t, - dataRow: t.BeginRow() + 1, // StructDescriptor's datarow is 2 (1-based) + basePositioner: basePositioner{tabler: t}, + dataRow: t.BeginRow() + 1, // StructDescriptor's datarow is 2 (1-based) }, } for _, field := range desc.Fields { @@ -227,26 +269,18 @@ func extractUnionTypeInfo(sheet *book.Sheet, typeName, parentFilename string, pa // unionFieldPositioner correctly maps positions for union type sheets where // cursor iterates over field columns within a specific union value row. type unionFieldPositioner struct { - tabler book.Tabler - valueRow int // 0-based row of current union value in tabler's coordinate - fieldStartCol int // 0-based column where Field1 starts + basePositioner + valueRow int // 0-based row of current union value in tabler's coordinate } func (p *unionFieldPositioner) Position(row, col int) string { // row param is unused since name/type/note are all in the same cell (different lines). - // col is the cursor (field index within this value), maps to actual column. - return p.tabler.Position(p.valueRow, p.fieldStartCol+col) -} - -// findFieldStartCol finds the 0-based column index of "Field1" in the header row. -func findFieldStartCol(t book.Tabler) int { - headerRow := t.GetRow(t.BeginRow()) // namerow=1, 0-based: BeginRow+0 - for i, cell := range headerRow { - if cell == colFieldPrefix+"1" { - return i - } + // col is the cursor (field index within this value), maps to "Field1", "Field2", ... via colIndex. + name := colFieldPrefix + strconv.Itoa(col+1) // 0-based col -> "Field1", "Field2", ... + if colIdx, ok := p.colIndex(name); ok { + return p.tabler.Position(p.valueRow, colIdx) } - return 0 // fallback + return "" } func parseUnionType(ws *internalpb.Worksheet, sheet *book.Sheet, parser book.SheetParser, gen *Generator, debugBookName, debugSheetName string) error { @@ -277,19 +311,18 @@ func parseUnionType(ws *internalpb.Worksheet, sheet *book.Sheet, parser book.She // create a book parser bp := newTableParser("union", "", "", gen) t := sheet.Tabler() - fieldStartCol := findFieldStartCol(t) shHeader := &tableHeader{ Header: &tableparser.Header{ NameRow: 1, TypeRow: 1, + NoteRow: 1, NameLine: 1, TypeLine: 2, NoteLine: 3, }, Positioner: &unionFieldPositioner{ - tabler: t, - valueRow: t.BeginRow() + 1 + i, // datarow=2 (1-based), i is the value index - fieldStartCol: fieldStartCol, + basePositioner: basePositioner{tabler: t}, + valueRow: t.BeginRow() + 1 + i, // datarow=2 (1-based), i is the value index }, nameRowData: value.Fields, typeRowData: value.Fields, diff --git a/internal/protogen/sheet_mode_test.go b/internal/protogen/sheet_mode_test.go index eb2a37e8..ccde5eb8 100644 --- a/internal/protogen/sheet_mode_test.go +++ b/internal/protogen/sheet_mode_test.go @@ -25,13 +25,13 @@ func TestVerticalPositioner_Position(t *testing.T) { }) p := &verticalPositioner{ - tabler: table, - dataRow: table.BeginRow() + 1, // datarow=2 (1-based), 0-based: 1 + basePositioner: basePositioner{tabler: table}, + dataRow: table.BeginRow() + 1, // datarow=2 (1-based), 0-based: 1 } tests := []struct { name string - row int // virtual header row index: 0=Name col, 1=Type col + row int // virtual header row index: 0=Name, 1=Type, 2=Note col int // cursor (field index) expect string }{ @@ -39,25 +39,96 @@ func TestVerticalPositioner_Position(t *testing.T) { name: "NameRow-cursor0", row: 0, col: 0, - expect: "A2", // dataRow(1)+cursor(0)=row1, col0=A => A2 + expect: "A2", // dataRow(1)+cursor(0)=row1, colMap["Name"]=0=A => A2 }, { name: "TypeRow-cursor0", row: 1, col: 0, - expect: "B2", // dataRow(1)+cursor(0)=row1, col1=B => B2 + expect: "B2", // dataRow(1)+cursor(0)=row1, colMap["Type"]=1=B => B2 }, { name: "NameRow-cursor4", row: 0, col: 4, - expect: "A6", // dataRow(1)+cursor(4)=row5, col0=A => A6 + expect: "A6", // dataRow(1)+cursor(4)=row5, colMap["Name"]=0=A => A6 }, { name: "TypeRow-cursor4", row: 1, col: 4, - expect: "B6", // dataRow(1)+cursor(4)=row5, col1=B => B6 + expect: "B6", // dataRow(1)+cursor(4)=row5, colMap["Type"]=1=B => B6 + }, + { + name: "NoteRow-missing-returns-empty", + row: 2, // Note column does not exist in colMap + col: 0, + expect: "", // should return empty string + }, + { + name: "negative-row-returns-empty", + row: -1, // e.g., NoteRow=0 means row-1=-1 + col: 0, + expect: "", // should return empty string + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := p.Position(tt.row, tt.col) + if got != tt.expect { + t.Errorf("verticalPositioner.Position(%d, %d) = %v, want %v", tt.row, tt.col, got, tt.expect) + } + }) + } +} + +func TestVerticalPositioner_Position_Disordered(t *testing.T) { + // Simulate a non-transposed struct type sheet with disordered columns: + // A(col0) B(col1) + // R1: Type Name <- header (columns swapped!) + // R2: int32 ID <- data row 0 + // R3: string Name <- data row 1 + table := book.NewTable([][]string{ + {"Type", "Name"}, // row 0 (header) + {"int32", "ID"}, // row 1 (data row 0) + {"string", "MyName"}, // row 2 (data row 1) + }) + + p := &verticalPositioner{ + basePositioner: basePositioner{tabler: table}, + dataRow: table.BeginRow() + 1, // datarow=2 (1-based), 0-based: 1 + } + + tests := []struct { + name string + row int + col int + expect string + }{ + { + name: "NameRow-cursor0", + row: 0, + col: 0, + expect: "B2", // dataRow(1)+cursor(0)=row1, colMap["Name"]=1=B => B2 + }, + { + name: "TypeRow-cursor0", + row: 1, + col: 0, + expect: "A2", // dataRow(1)+cursor(0)=row1, colMap["Type"]=0=A => A2 + }, + { + name: "NameRow-cursor1", + row: 0, + col: 1, + expect: "B3", // dataRow(1)+cursor(1)=row2, colMap["Name"]=1=B => B3 + }, + { + name: "TypeRow-cursor1", + row: 1, + col: 1, + expect: "A3", // dataRow(1)+cursor(1)=row2, colMap["Type"]=0=A => A3 }, } @@ -94,8 +165,8 @@ func TestVerticalPositioner_Position_Transposed(t *testing.T) { transposed := table.Transpose() p := &verticalPositioner{ - tabler: transposed, - dataRow: transposed.BeginRow() + 1, // datarow=2 (1-based), 0-based in transposed: 1 + basePositioner: basePositioner{tabler: transposed}, + dataRow: transposed.BeginRow() + 1, // datarow=2 (1-based), 0-based in transposed: 1 } tests := []struct { @@ -141,71 +212,136 @@ func TestVerticalPositioner_Position_Transposed(t *testing.T) { } func TestUnionFieldPositioner_Position(t *testing.T) { - // Simulate a non-transposed union type sheet: + // Simulate a non-transposed union type sheet with ordered columns: // A(col0) B(col1) C(col2) D(col3) E(col4) F(col5) - // R1: Name Alias Type Field1 Field2 Field3 <- header + // R1: Name Alias Type Field1 Field2 Field3 <- header // R2: Value0 Alias0 Type0 <- value 0 // R3: Value1 Alias1 Type1 <- value 1 // ... // R228: Value226 Alias226 Type226 <- value 226 - table := book.NewTable(make([][]string, 228)) // 228 rows + rows := make([][]string, 228) + rows[0] = []string{"Name", "Alias", "Type", "Field1", "Field2", "Field3"} // header row + table := book.NewTable(rows) tests := []struct { - name string - valueRow int - fieldStartCol int - row int // unused in unionFieldPositioner - col int // cursor (field index) - expect string + name string + valueRow int + row int // unused in unionFieldPositioner + col int // cursor (field index) + expect string }{ { - name: "value0-field0", - valueRow: 1, // datarow=2 (1-based), BeginRow(0)+1+0=1 - fieldStartCol: 3, // Field1 at col D - row: 0, - col: 0, - expect: "D2", // Position(1, 3) => D2 + name: "value0-field0", + valueRow: 1, // datarow=2 (1-based), BeginRow(0)+1+0=1 + row: 0, + col: 0, + expect: "D2", // col=0 -> "Field1" -> colMap["Field1"]=3 -> Position(1, 3) => D2 + }, + { + name: "value0-field2", + valueRow: 1, + row: 0, + col: 2, + expect: "F2", // col=2 -> "Field3" -> colMap["Field3"]=5 -> Position(1, 5) => F2 }, { - name: "value0-field2", - valueRow: 1, - fieldStartCol: 3, - row: 0, - col: 2, - expect: "F2", // Position(1, 5) => F2 + name: "value226-field0-D228", + valueRow: 227, // BeginRow(0)+1+226=227 + row: 0, + col: 0, + expect: "D228", // col=0 -> "Field1" -> colMap["Field1"]=3 -> Position(227, 3) => D228 }, { - name: "value226-field0-D228", - valueRow: 227, // BeginRow(0)+1+226=227 - fieldStartCol: 3, - row: 0, - col: 0, - expect: "D228", // Position(227, 3) => D228 + name: "value226-field1-E228", + valueRow: 227, + row: 0, + col: 1, + expect: "E228", // col=1 -> "Field2" -> colMap["Field2"]=4 -> Position(227, 4) => E228 }, { - name: "value226-field1-E228", - valueRow: 227, - fieldStartCol: 3, - row: 0, - col: 1, - expect: "E228", // Position(227, 4) => E228 + name: "row-param-ignored", + valueRow: 227, + row: 1, // different row param, should still produce same result + col: 0, + expect: "D228", // row param is ignored, col=0 -> "Field1" -> Position(227, 3) => D228 }, { - name: "row-param-ignored", - valueRow: 227, - fieldStartCol: 3, - row: 1, // different row param, should still produce same result - col: 0, - expect: "D228", // row param is ignored, Position(227, 3) => D228 + name: "missing-field-returns-empty", + valueRow: 1, + row: 0, + col: 3, // col=3 -> "Field4" -> not in colMap -> "" + expect: "", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { p := &unionFieldPositioner{ - tabler: table, - valueRow: tt.valueRow, - fieldStartCol: tt.fieldStartCol, + basePositioner: basePositioner{tabler: table}, + valueRow: tt.valueRow, + } + got := p.Position(tt.row, tt.col) + if got != tt.expect { + t.Errorf("unionFieldPositioner.Position(%d, %d) = %v, want %v", tt.row, tt.col, got, tt.expect) + } + }) + } +} + +func TestUnionFieldPositioner_Position_Disordered(t *testing.T) { + // Simulate a non-transposed union type sheet with disordered columns: + // A(col0) B(col1) C(col2) D(col3) E(col4) + // R1: Field3 Name Field2 Alias Field1 <- header (disordered!) + // R2: Value0 Alias0 <- value 0 + // R3: Value1 Alias1 <- value 1 + table := book.NewTable([][]string{ + {"Field3", "Name", "Field2", "Alias", "Field1"}, + {"cell00", "Value0", "cell01", "Alias0", "cell02"}, + {"cell10", "Value1", "cell11", "Alias1", "cell12"}, + }) + + tests := []struct { + name string + valueRow int + row int + col int + expect string + }{ + { + name: "value0-field0-maps-to-Field1-col4", + valueRow: 1, + row: 0, + col: 0, + expect: "E2", // col=0 -> "Field1" -> colMap["Field1"]=4 -> Position(1, 4) => E2 + }, + { + name: "value0-field1-maps-to-Field2-col2", + valueRow: 1, + row: 0, + col: 1, + expect: "C2", // col=1 -> "Field2" -> colMap["Field2"]=2 -> Position(1, 2) => C2 + }, + { + name: "value0-field2-maps-to-Field3-col0", + valueRow: 1, + row: 0, + col: 2, + expect: "A2", // col=2 -> "Field3" -> colMap["Field3"]=0 -> Position(1, 0) => A2 + }, + { + name: "value1-field0-maps-to-Field1-col4", + valueRow: 2, + row: 0, + col: 0, + expect: "E3", // col=0 -> "Field1" -> colMap["Field1"]=4 -> Position(2, 4) => E3 + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + p := &unionFieldPositioner{ + basePositioner: basePositioner{tabler: table}, + valueRow: tt.valueRow, } got := p.Position(tt.row, tt.col) if got != tt.expect { @@ -243,38 +379,34 @@ func TestUnionFieldPositioner_Position_Transposed(t *testing.T) { transposed := table.Transpose() tests := []struct { - name string - valueRow int - fieldStartCol int - row int - col int - expect string + name string + valueRow int + row int + col int + expect string }{ { - name: "value0-field0", - valueRow: 1, // BeginRow(0)+1+0=1 - fieldStartCol: 3, // Field1 at virtual col 3 - row: 0, - col: 0, - // transposed.Position(1, 3) => table.Position(3, 1) => B4 + name: "value0-field0", + valueRow: 1, // BeginRow(0)+1+0=1 + row: 0, + col: 0, + // col=0 -> "Field1" -> colMap["Field1"]=3 -> transposed.Position(1, 3) => table.Position(3, 1) => B4 expect: "B4", }, { - name: "value0-field1", - valueRow: 1, - fieldStartCol: 3, - row: 0, - col: 1, - // transposed.Position(1, 4) => table.Position(4, 1) => B5 + name: "value0-field1", + valueRow: 1, + row: 0, + col: 1, + // col=1 -> "Field2" -> colMap["Field2"]=4 -> transposed.Position(1, 4) => table.Position(4, 1) => B5 expect: "B5", }, { - name: "value1-field0", - valueRow: 2, // BeginRow(0)+1+1=2 - fieldStartCol: 3, - row: 0, - col: 0, - // transposed.Position(2, 3) => table.Position(3, 2) => C4 + name: "value1-field0", + valueRow: 2, // BeginRow(0)+1+1=2 + row: 0, + col: 0, + // col=0 -> "Field1" -> colMap["Field1"]=3 -> transposed.Position(2, 3) => table.Position(3, 2) => C4 expect: "C4", }, } @@ -282,9 +414,8 @@ func TestUnionFieldPositioner_Position_Transposed(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { p := &unionFieldPositioner{ - tabler: transposed, - valueRow: tt.valueRow, - fieldStartCol: tt.fieldStartCol, + basePositioner: basePositioner{tabler: transposed}, + valueRow: tt.valueRow, } got := p.Position(tt.row, tt.col) if got != tt.expect { @@ -294,72 +425,85 @@ func TestUnionFieldPositioner_Position_Transposed(t *testing.T) { } } -func TestFindFieldStartCol(t *testing.T) { +func TestBasePositioner_ColIndex(t *testing.T) { tests := []struct { name string tabler book.Tabler - expect int + expect map[string]int }{ { - name: "standard-union-header-with-type", + name: "struct-header-Name-Type", tabler: book.NewTable([][]string{ - {"Name", "Alias", "Type", "Field1", "Field2"}, - {"v1", "a1", "t1", "f1\nint32", "f2\nstring"}, + {"Name", "Type"}, + {"ID", "uint32"}, }), - expect: 3, // Field1 at col D (index 3) + expect: map[string]int{"Name": 0, "Type": 1}, }, { - name: "union-header-without-number-and-type", + name: "struct-header-Name-Type-Note", tabler: book.NewTable([][]string{ - {"Name", "Alias", "Field1", "Field2"}, - {"v1", "a1", "f1\nint32", "f2\nstring"}, + {"Name", "Type", "Note"}, + {"ID", "uint32", "some note"}, }), - expect: 2, // Field1 at col C (index 2) + expect: map[string]int{"Name": 0, "Type": 1, "Note": 2}, }, { - name: "union-header-with-number", + name: "struct-header-disordered", tabler: book.NewTable([][]string{ - {"Number", "Name", "Alias", "Type", "Field1", "Field2"}, - {"1", "v1", "a1", "t1", "f1\nint32", "f2\nstring"}, + {"Note", "Type", "Name"}, + {"some note", "int32", "ID"}, }), - expect: 4, // Field1 at col E (index 4) + expect: map[string]int{"Note": 0, "Type": 1, "Name": 2}, }, { - name: "no-field-columns-fallback", + name: "union-header", tabler: book.NewTable([][]string{ - {"Name", "Alias", "Type"}, - {"v1", "a1", "t1"}, + {"Name", "Alias", "Type", "Field1", "Field2", "Field3"}, + {"v1", "a1", "t1", "f1", "f2", "f3"}, }), - expect: 0, // fallback to 0 + expect: map[string]int{"Name": 0, "Alias": 1, "Type": 2, "Field1": 3, "Field2": 4, "Field3": 5}, }, { - name: "transposed-union-header", + name: "union-header-disordered", tabler: book.NewTable([][]string{ - // Underlying table (before transpose): - // A B C - // R1: Name v1 v2 - // R2: Alias a1 a2 - // R3: Type t1 t2 - // R4: Field1 f1 f3 - // R5: Field2 f2 f4 - {"Name", "v1", "v2"}, - {"Alias", "a1", "a2"}, - {"Type", "t1", "t2"}, - {"Field1", "f1", "f3"}, - {"Field2", "f2", "f4"}, + {"Field3", "Name", "Field2", "Alias", "Field1"}, + {"f3", "v1", "f2", "a1", "f1"}, + }), + expect: map[string]int{"Field3": 0, "Name": 1, "Field2": 2, "Alias": 3, "Field1": 4}, + }, + { + name: "empty-cells-skipped", + tabler: book.NewTable([][]string{ + {"Name", "", "Type"}, + {"ID", "", "uint32"}, + }), + expect: map[string]int{"Name": 0, "Type": 2}, + }, + { + name: "transposed", + tabler: book.NewTable([][]string{ + {"Name", "ID", "Num"}, + {"Type", "uint32", "int32"}, }).Transpose(), - // Transposed virtual header row (GetRow(BeginRow=0)): - // GetRow(0) => table.getCol(0) => ["Name", "Alias", "Type", "Field1", "Field2"] - // Field1 at index 3 - expect: 3, + // Transposed virtual header row: GetRow(0) => ["Name", "Type"] + expect: map[string]int{"Name": 0, "Type": 1}, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := findFieldStartCol(tt.tabler) - if got != tt.expect { - t.Errorf("findFieldStartCol() = %v, want %v", got, tt.expect) + bp := &basePositioner{tabler: tt.tabler} + for k, v := range tt.expect { + gotV, ok := bp.colIndex(k) + if !ok { + t.Errorf("basePositioner.colIndex(%q) not found, want %v", k, v) + } else if gotV != v { + t.Errorf("basePositioner.colIndex(%q) = %v, want %v", k, gotV, v) + } + } + // Verify that a non-existent column returns false + if _, ok := bp.colIndex("NonExistent"); ok { + t.Errorf("basePositioner.colIndex(%q) should return false", "NonExistent") } }) } From f481501600f4aa9a1cbf95fdb392e70b975bd768 Mon Sep 17 00:00:00 2001 From: wenchy Date: Tue, 7 Apr 2026 21:25:14 +0800 Subject: [PATCH 3/3] refactor(protogen): improve comments and logic clarity for positioners --- internal/protogen/sheet_mode.go | 118 ++++++++++++++++----------- internal/protogen/sheet_mode_test.go | 12 +-- 2 files changed, 75 insertions(+), 55 deletions(-) diff --git a/internal/protogen/sheet_mode.go b/internal/protogen/sheet_mode.go index 36b1a7ed..2ef931d6 100644 --- a/internal/protogen/sheet_mode.go +++ b/internal/protogen/sheet_mode.go @@ -266,14 +266,16 @@ func extractUnionTypeInfo(sheet *book.Sheet, typeName, parentFilename string, pa return nil } -// unionFieldPositioner correctly maps positions for union type sheets where -// cursor iterates over field columns within a specific union value row. -type unionFieldPositioner struct { +// unionValueFieldPositioner resolves cell positions for a single union value row. +// The col cursor maps to the physical column of Field1, Field2, ... FieldN in order, +// while the row parameter is ignored because name, type, and note are stored as +// separate lines within the same cell rather than in separate rows. +type unionValueFieldPositioner struct { basePositioner valueRow int // 0-based row of current union value in tabler's coordinate } -func (p *unionFieldPositioner) Position(row, col int) string { +func (p *unionValueFieldPositioner) Position(row, col int) string { // row param is unused since name/type/note are all in the same cell (different lines). // col is the cursor (field index within this value), maps to "Field1", "Field2", ... via colIndex. name := colFieldPrefix + strconv.Itoa(col+1) // 0-based col -> "Field1", "Field2", ... @@ -289,59 +291,77 @@ func parseUnionType(ws *internalpb.Worksheet, sheet *book.Sheet, parser book.She return err } + // bp and t are shared across all union values; create them once outside the loop. + bp := newTableParser("union-fields", "", "", gen) + t := sheet.Tabler() + for i, value := range desc.Values { - number := int32(i + 1) - if value.Number != nil { - number = *value.Number - } - field := &internalpb.Field{ - Number: number, - Name: strings.TrimSpace(value.Name), - Alias: strings.TrimSpace(value.Alias), + field, err := newUnionField(i, value, gen, sheet.Name) + if err != nil { + return err } - if typ := strings.TrimSpace(value.Type); typ != "" { - typeDesc, err := parseTypeDescriptor(gen.typeInfos, typ) - if err != nil { - return xerrors.Wrapf(err, "failed to parse union type %s of sheet: %s", typ, sheet.Name) - } - field.Type = typeDesc.Name - field.FullType = typeDesc.FullName + if err := parseUnionValueFields(field, value, bp, t, i, debugBookName, debugSheetName); err != nil { + return err } + ws.Fields = append(ws.Fields, field) + } + return nil +} - // create a book parser - bp := newTableParser("union", "", "", gen) - t := sheet.Tabler() - shHeader := &tableHeader{ - Header: &tableparser.Header{ - NameRow: 1, - TypeRow: 1, - NoteRow: 1, - NameLine: 1, - TypeLine: 2, - NoteLine: 3, - }, - Positioner: &unionFieldPositioner{ - basePositioner: basePositioner{tabler: t}, - valueRow: t.BeginRow() + 1 + i, // datarow=2 (1-based), i is the value index - }, - nameRowData: value.Fields, - typeRowData: value.Fields, - noteRowData: value.Fields, +// newUnionField builds the top-level Field for a single union value, resolving +// its optional type descriptor when a type string is present. +func newUnionField(i int, value *internalpb.UnionDescriptor_Value, gen *Generator, sheetName string) (*internalpb.Field, error) { + number := int32(i + 1) + if value.Number != nil { + number = *value.Number + } + field := &internalpb.Field{ + Number: number, + Name: strings.TrimSpace(value.Name), + Alias: strings.TrimSpace(value.Alias), + } + if typ := strings.TrimSpace(value.Type); typ != "" { + typeDesc, err := parseTypeDescriptor(gen.typeInfos, typ) + if err != nil { + return nil, xerrors.Wrapf(err, "failed to parse union type %s of sheet: %s", typ, sheetName) } + field.Type = typeDesc.Name + field.FullType = typeDesc.FullName + } + return field, nil +} + +// parseUnionValueFields parses the Field1...N columns of a union value row into +// the sub-fields of the union oneof message, appending them to field.Fields. +func parseUnionValueFields(field *internalpb.Field, value *internalpb.UnionDescriptor_Value, bp *tableParser, t book.Tabler, i int, debugBookName, debugSheetName string) error { + shHeader := &tableHeader{ + Header: &tableparser.Header{ + NameRow: 1, + TypeRow: 1, + NoteRow: 1, + NameLine: 1, + TypeLine: 2, + NoteLine: 3, + }, + Positioner: &unionValueFieldPositioner{ + basePositioner: basePositioner{tabler: t}, + valueRow: t.BeginRow() + 1 + i, // datarow=2 (1-based), i is the value index + }, + nameRowData: value.Fields, + typeRowData: value.Fields, + noteRowData: value.Fields, + } + for cursor := 0; cursor < len(shHeader.nameRowData); cursor++ { + subField := &internalpb.Field{} var parsed bool var err error - for cursor := 0; cursor < len(shHeader.nameRowData); cursor++ { - subField := &internalpb.Field{} - cursor, parsed, err = bp.parseField(subField, shHeader, cursor, "", "", tableparser.Mode(tableaupb.Mode_MODE_UNION_TYPE)) - if err != nil { - return wrapDebugErr(err, debugBookName, debugSheetName, shHeader, cursor) - } - if parsed { - field.Fields = append(field.Fields, subField) - } + cursor, parsed, err = bp.parseField(subField, shHeader, cursor, "", "", tableparser.Mode(tableaupb.Mode_MODE_UNION_TYPE)) + if err != nil { + return wrapDebugErr(err, debugBookName, debugSheetName, shHeader, cursor) + } + if parsed { + field.Fields = append(field.Fields, subField) } - - ws.Fields = append(ws.Fields, field) } return nil } diff --git a/internal/protogen/sheet_mode_test.go b/internal/protogen/sheet_mode_test.go index ccde5eb8..52f3c651 100644 --- a/internal/protogen/sheet_mode_test.go +++ b/internal/protogen/sheet_mode_test.go @@ -211,7 +211,7 @@ func TestVerticalPositioner_Position_Transposed(t *testing.T) { } } -func TestUnionFieldPositioner_Position(t *testing.T) { +func TestUnionValueFieldPositioner_Position(t *testing.T) { // Simulate a non-transposed union type sheet with ordered columns: // A(col0) B(col1) C(col2) D(col3) E(col4) F(col5) // R1: Name Alias Type Field1 Field2 Field3 <- header @@ -276,7 +276,7 @@ func TestUnionFieldPositioner_Position(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - p := &unionFieldPositioner{ + p := &unionValueFieldPositioner{ basePositioner: basePositioner{tabler: table}, valueRow: tt.valueRow, } @@ -288,7 +288,7 @@ func TestUnionFieldPositioner_Position(t *testing.T) { } } -func TestUnionFieldPositioner_Position_Disordered(t *testing.T) { +func TestUnionValueFieldPositioner_Position_Disordered(t *testing.T) { // Simulate a non-transposed union type sheet with disordered columns: // A(col0) B(col1) C(col2) D(col3) E(col4) // R1: Field3 Name Field2 Alias Field1 <- header (disordered!) @@ -339,7 +339,7 @@ func TestUnionFieldPositioner_Position_Disordered(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - p := &unionFieldPositioner{ + p := &unionValueFieldPositioner{ basePositioner: basePositioner{tabler: table}, valueRow: tt.valueRow, } @@ -351,7 +351,7 @@ func TestUnionFieldPositioner_Position_Disordered(t *testing.T) { } } -func TestUnionFieldPositioner_Position_Transposed(t *testing.T) { +func TestUnionValueFieldPositioner_Position_Transposed(t *testing.T) { // Simulate a transposed union type sheet: // The underlying table is: // A(col0) B(col1) C(col2) ... @@ -413,7 +413,7 @@ func TestUnionFieldPositioner_Position_Transposed(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - p := &unionFieldPositioner{ + p := &unionValueFieldPositioner{ basePositioner: basePositioner{tabler: transposed}, valueRow: tt.valueRow, }