Skip to content

Beta#1

Merged
VarusHsu merged 7 commits into
masterfrom
beta
Apr 29, 2026
Merged

Beta#1
VarusHsu merged 7 commits into
masterfrom
beta

Conversation

@VarusHsu

Copy link
Copy Markdown
Owner

No description provided.

@VarusHsu

Copy link
Copy Markdown
Owner Author

@copilot

@codecov

codecov Bot commented Apr 27, 2026

Copy link
Copy Markdown

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

Copilot AI commented Apr 27, 2026

Copy link
Copy Markdown

@copilot

It looks like your comment is empty. Could you please let me know what you'd like me to help with?

@VarusHsu

Copy link
Copy Markdown
Owner Author

@copilot review this pr

Copilot AI commented Apr 27, 2026

Copy link
Copy Markdown

@copilot review this pr

Here's my review of the PR:


Summary

The PR adds symmetric read APIs (ReadExcelToModels, ReadExcelToModelsFromBytesBuffer, ReadExcelToMaps, ReadExcelToMapsFromBytesBuffer) to complement the existing write side, along with strict-mode header validation and functional options. The implementation is clean and well-tested. All existing and new tests pass. A few findings below:


Issues

1. Dead code in sheetNameFromType (read.go:371–377)

The second interface assertion is unreachable. In Go, a value-receiver method on T is automatically promoted to *T, so reflect.New(modelType).Interface().(SheetModel) (which yields *T) already succeeds for both value- and pointer-receiver implementations. The second branch reflect.New(modelType).Elem().Interface().(SheetModel) can never be reached.

// The second block is dead code — remove it:
if model, ok := reflect.New(modelType).Elem().Interface().(SheetModel); ok { ... }

2. String values are assigned from raw, not trimmed (read.go:486)

All numeric and bool cases parse from trimmed, but strings set from the untrimmed raw. This is probably intentional to preserve leading/trailing spaces in string fields, but it's silently inconsistent. At minimum worth a short comment.

3. write.go's appendRow does not skip "-" tagged fields in the header row (write.go:320–330)

When excel_header:"-" or excelorm:"-" is used, setNoDataSheetHeaders correctly skips those fields. But appendRow's header-writing loop has no such check — it will write "-" as a literal column header. On the read side getReadModelMeta correctly skips "-" fields. This means a round-trip write→read will silently produce a header mismatch (an unrecognised "-" column in non-strict mode, or a validation error in strict mode). This is a pre-existing issue in the write path, but now that the read path exists the asymmetry has real consequences.

4. validateStrictModelHeaders rebuilds a set it already has (read.go:414)

The function iterates over headers to build a local headerSet, but meta.headerSet already holds the model's headers with the same membership. The local set is actually built from the Excel headers (not model headers), so the reverse check at line 421 is correct. No bug — just the variable name headerSet inside the function shadows the concept a bit. No action needed.

5. Minor: no test for header-only sheet (1 row, no data)

The empty-sheet path (len(rows) == 0) is tested, but there is no test for a sheet that has only a header row and zero data rows. The code handles it correctly (capacity len(rows)-1 = 0), but a test would confirm the behaviour explicitly.


What's good

  • Functional-option pattern mirrors the write side consistently.
  • sync.Map metadata cache avoids repeated reflection on hot paths — confirmed by the benchmark tests.
  • Error messages include sheet name, row, and column for easy debugging.
  • isEmptyRow correctly skips blank rows, and safeCellValue handles short rows safely.
  • Both *[]T and []*T slice shapes are supported and tested.

Copilot AI 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.

Pull request overview

This PR introduces the core “beta” read/write APIs for the excelorm package, enabling Excel generation (to file or buffer) and parsing Excel sheets back into typed models or header/value maps.

Changes:

  • Added Excel write APIs (WriteExcelSaveAs, WriteExcelAsBytesBuffer) with configurable formatting and output options.
  • Added Excel read APIs (ReadExcelToModels, ReadExcelToMaps, and buffer variants) including strict header validation and parsing options.
  • Added comprehensive tests/benchmarks and updated documentation for the new read APIs.

Reviewed changes

Copilot reviewed 5 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
write.go Implements Excel writing, options, reflection-based row append, and header helpers.
write_test.go Adds unit tests covering write success/error cases and option behaviors.
write_bench_test.go Adds benchmarks for write throughput across modes (pointer, multi-sheet, headless, etc.).
read.go Implements Excel reading into models/maps, strict header validation, and parsing options with caching.
read_test.go Adds tests for reading into models/maps, strict mode, and option parsing behaviors.
read_bench_test.go Adds benchmarks for read performance for models/maps including strict mode.
readme.md Documents the new read APIs, strict mode, and read options.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread read.go
@VarusHsu VarusHsu merged commit c264498 into master Apr 29, 2026
2 checks passed
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.

3 participants