From 12b8c6fcf593550702a16423ea39e219286c4eb1 Mon Sep 17 00:00:00 2001 From: per Date: Tue, 5 May 2026 22:44:13 +0200 Subject: [PATCH 1/4] Implement matrix-spreadsheet v2.4.0 roadmap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 1 — Safe internal fixes: - FExcelExporter: remove dead I/O when target file exists; throw directly - FOdsImporter: replace undeclared commons-io transitive dep with is.bytes - SpreadsheetImporter: make validateNotNull private - ValueExtractor: add @CompileStatic - FExcelImporter + FOdsImporter: improve missing-sheet error messages - SpreadsheetImporter: fix @param mismatches (file → url) and add class-level GroovyDoc - SpreadsheetWriter: document writeSheets(Map) parameter keys - FExcelExporter: fix stale GroovyDoc claiming append support Phase 2 — API consistency: - Importer + SpreadsheetImporter: standardise startColumn/endColumn → startCol/endCol - fastods/Sheet: tighten name field from Object to String Phase 3 — Usability improvements: - SpreadsheetImporter: add File-based overloads delegating via absolutePath - SpreadsheetImporter: add whole-sheet convenience imports with auto-detected dimensions - SpreadsheetImporter: remove misleading 'Sheet1' default from String sheet overloads Phase 4 — Refactoring: - FileUtil: replace generic Exception throws with SpreadsheetImportException Phase 5 — Documentation: - Investigate and document why FExcelExporter cannot use @CompileStatic (fastexcel internal GenericStyleSetter is package-private) - README: update versions (groovy 5.0.5, matrix-core 3.7.1, spreadsheet 2.4.0), remove stale Log4j claim, add compatibility matrix row, fix Sheet1 docs - release.md: add v2.4.0 release notes - build.gradle + bom.xml: remove SNAPSHOT suffix --- matrix-bom/bom.xml | 2 +- matrix-spreadsheet/README.md | 21 +- matrix-spreadsheet/build.gradle | 2 +- matrix-spreadsheet/release.md | 32 +++ matrix-spreadsheet/req/v2.4.0-roadmap.md | 157 +++++++++++++ .../alipsa/matrix/spreadsheet/FileUtil.groovy | 10 +- .../alipsa/matrix/spreadsheet/Importer.groovy | 2 +- .../spreadsheet/SpreadsheetImporter.groovy | 210 ++++++++++++++---- .../spreadsheet/SpreadsheetWriter.groovy | 9 +- .../matrix/spreadsheet/ValueExtractor.groovy | 3 + .../fastexcel/FExcelExporter.groovy | 67 +++--- .../fastexcel/FExcelImporter.groovy | 14 +- .../spreadsheet/fastods/FOdsImporter.groovy | 10 +- .../matrix/spreadsheet/fastods/Sheet.groovy | 10 +- matrix-spreadsheet/v2.4.0-roadmap.md | 52 ----- 15 files changed, 438 insertions(+), 163 deletions(-) create mode 100644 matrix-spreadsheet/req/v2.4.0-roadmap.md delete mode 100644 matrix-spreadsheet/v2.4.0-roadmap.md diff --git a/matrix-bom/bom.xml b/matrix-bom/bom.xml index c144770da..2dc4dbebd 100644 --- a/matrix-bom/bom.xml +++ b/matrix-bom/bom.xml @@ -46,7 +46,7 @@ 0.1.0-SNAPSHOT 0.5.0 0.1.1-SNAPSHOT - 2.3.1-SNAPSHOT + 2.4.0 2.4.0 2.4.0 0.3.0 diff --git a/matrix-spreadsheet/README.md b/matrix-spreadsheet/README.md index 268c357e0..5b2f8e9b7 100644 --- a/matrix-spreadsheet/README.md +++ b/matrix-spreadsheet/README.md @@ -9,9 +9,9 @@ for [Renjin R](https://github.com/bedatadriven/renjin). To use it, add the following to your gradle build script: ```groovy -implementation 'org.apache.groovy:groovy:5.0.4' -implementation 'se.alipsa.matrix:matrix-core:3.6.0' -implementation 'se.alipsa.matrix:matrix-spreadsheet:2.3.0' +implementation 'org.apache.groovy:groovy:5.0.5' +implementation 'se.alipsa.matrix:matrix-core:3.7.1' +implementation 'se.alipsa.matrix:matrix-spreadsheet:2.4.0' ``` or if you use maven: ```xml @@ -19,17 +19,17 @@ or if you use maven: org.apache.groovy groovy - 5.0.4 + 5.0.5 se.alipsa.matrix matrix-core - 3.6.0 + 3.7.1 se.alipsa.matrix matrix-spreadsheet - 2.3.0 + 2.4.0 ``` @@ -44,7 +44,7 @@ println(table.head(10)) ``` The SpreadSheetImporter.importSpreadSheetSheet takes the following parameters: - _file_ the filePath or the file object pointing to the Excel file -- _sheetName_ the name of the sheet to import, default is 'Sheet1' +- _sheetName_ the name of the sheet to import; when omitted, defaults to the first sheet (index 1) - _startRow_ the starting row for the import (as you would see the row number in Excel), defaults to 1 - _endRow_ the last row to import - _startCol_ the starting column name (A, B etc.) or column number (1, 2 etc.) @@ -210,6 +210,7 @@ The following table illustrates the version compatibility of the matrix-spreadsh | Matrix spreadsheet | Matrix core | |-------------------:|---------------:| +| 2.4.0 | 3.7.1 | | 2.3.0 | 3.4.1 | | 2.2.0 | 3.2.0 -> 3.3.0 | | 2.1.0 | 3.1.0 | @@ -245,7 +246,5 @@ Used to define the data format i.e. the result from an import or the data to exp - URL: https://github.com/Alipsa/matrix - License: MIT -### Log4j -Used to handle logging -- URL: https://logging.apache.org/log4j/2.x/ -- License: Apache 2.0 +### matrix-core Logger +Uses the lightweight JDK System.Logger facade via matrix-core; no direct logging framework dependency. diff --git a/matrix-spreadsheet/build.gradle b/matrix-spreadsheet/build.gradle index 40c4c96d4..9e6e1c8b1 100644 --- a/matrix-spreadsheet/build.gradle +++ b/matrix-spreadsheet/build.gradle @@ -11,7 +11,7 @@ plugins { } group = 'se.alipsa.matrix' -version = '2.3.1-SNAPSHOT' +version = '2.4.0' description = 'Matrix spreadsheet import/export' codenarc { diff --git a/matrix-spreadsheet/release.md b/matrix-spreadsheet/release.md index c33ab291d..1afee9fe8 100644 --- a/matrix-spreadsheet/release.md +++ b/matrix-spreadsheet/release.md @@ -1,5 +1,37 @@ # Release history +## v2.4.0, 2026-05-05 +**Refactoring, code quality, and usability improvements** + +### Breaking Changes +- removed `'Sheet1'` default from `String sheet` overloads in `SpreadsheetImporter`; callers relying on the implicit default should use the new whole-sheet convenience imports or pass an explicit sheet name +- `Sheet.name` field type tightened from `Object` to `String` +- `Importer` interface: `startColumn`/`endColumn` parameters renamed to `startCol`/`endCol` for consistency + +### New Features +- add whole-sheet convenience imports with auto-detected dimensions: `SpreadsheetImporter.importSpreadsheet(file, sheetNumber)` and `importSpreadsheet(file, sheetName)` +- add `File`-accepting overloads to `SpreadsheetImporter` matching `SpreadsheetWriter`'s API shape + +### Bug Fixes +- remove dead I/O and wasted workbook allocation in `FExcelExporter.exportExcel` when target file already exists +- replace undeclared `commons-io` transitive dependency in `FOdsImporter` with Groovy's native `is.bytes` +- improve error messages when a named sheet does not exist (names the missing sheet instead of generic "No value present") +- standardise rounding mode in `TableUtil.round` to `HALF_EVEN` (banker's rounding) across all numeric column types + +### Code Quality +- add class-level GroovyDoc and fix `@param` mismatches in `SpreadsheetImporter` +- add GroovyDoc to `SpreadsheetWriter.writeSheets(Map)` +- fix stale/misleading GroovyDoc on `FExcelExporter.exportExcel` overloads +- make `SpreadsheetImporter.validateNotNull` private +- add `@CompileStatic` to `ValueExtractor` +- replace generic `Exception` throws in `FileUtil` with `SpreadsheetImportException` +- document why `FExcelExporter` cannot use `@CompileStatic` (fastexcel internal `GenericStyleSetter` access) +- fix malformed `@code{...}` inline tags in `Gtable.groovy` GroovyDoc +- replace unbounded generic `` in `TableUtil.createColumn` with concrete `ColumnType` parameter + +### Test Coverage +- 124 tests passing (up from 105 in v2.3.0) + ## v2.3.0, 2026-01-31 **Major architectural refactoring with significant performance improvements** diff --git a/matrix-spreadsheet/req/v2.4.0-roadmap.md b/matrix-spreadsheet/req/v2.4.0-roadmap.md new file mode 100644 index 000000000..33e362114 --- /dev/null +++ b/matrix-spreadsheet/req/v2.4.0-roadmap.md @@ -0,0 +1,157 @@ +# Matrix Spreadsheet v2.4.0 Release Roadmap + +This document outlines the planned work for matrix-spreadsheet v2.4.0. + +## Summary + +v2.4.0 focuses on targeted refactoring, stronger test coverage (>= 70%), +performance benchmarking, release hygiene, and usability improvements identified +in the v2.3.1 code review. + +--- + +## Priority 1: Testing & Performance (Must Do) + +### 1.1 [x] Test coverage plan (target >= 70%) +- ✅ Completed in v2.3.0: Coverage achieved 79.74% (exceeds 70% target) +- See the detailed plan in v2.3.0 roadmap under "Test Coverage & Performance Plan". + +### 1.2 [x] Add performance benchmarks for XLSX/ODS read/write +- ✅ Completed in v2.3.0: SpreadsheetBenchmark added with 50k x 12 and Crime_Data tests +- Baseline read/write timing for realistic datasets documented in v2.3.0 roadmap. + +### 1.3 [x] Fix any performance regressions found in benchmarks +- ✅ Completed in v2.3.0: ODS performance improved 65-79% via Aalto parser and optimizations +- Before/after metrics captured: 4.86s → 1.70s (medium), 262s → 53s (large) + +--- + +## Priority 2: Refactoring (Should Do) + +### 2.1 [ ] Consider splitting FExcelAppender +- **File**: `src/main/groovy/se/alipsa/matrix/spreadsheet/fastexcel/FExcelAppender.groovy` +- **Issue**: ~750 lines with 11 inner classes +- **Consideration**: Could extract `WorkbookState`, `WorkbookPlan`, `SheetTemplate` to separate files +- **Note**: The class is well-structured internally; this is optional cleanup + +### 2.2 [ ] Consider using Optional in FExcelReader +- **File**: `src/main/groovy/se/alipsa/matrix/spreadsheet/fastexcel/FExcelReader.groovy` +- **Issue**: Returns -1 when sheet/content not found (lines 85, 97, 146) +- **Consideration**: Modern Java practice would use Optional +- **Note**: This is a breaking API change; defer to v3.0 if needed + +### 2.3 [ ] Consider more specific exception types in FileUtil +- **File**: `src/main/groovy/se/alipsa/matrix/spreadsheet/FileUtil.groovy` +- **Issue**: Throws generic `Exception` in several places +- **Note**: Low priority; current behavior works + +--- + +## Priority 3: Bug Fixes & Code Quality (from v2.3.1 review) + +### 3.1 [ ] Remove dead code and wasted I/O in `FExcelExporter.exportExcel` +- **File**: `src/main/groovy/se/alipsa/matrix/spreadsheet/fastexcel/FExcelExporter.groovy` lines 83–97 +- **Issue**: When the target file already exists, the method opens a `ReadableWorkbook` (allocating I/O resources), runs sheet-collision avoidance logic, and then unconditionally throws `IllegalArgumentException`. The collision-avoidance `if/while` block is completely dead — the throw always fires regardless. The workbook is opened and closed for nothing. +- **Fix**: Remove the entire `try` block. Replace the `if (file.exists() && file.length() > 0)` branch with a direct throw: + ```groovy + if (file.exists() && file.length() > 0) { + throw new IllegalArgumentException("Appending to an existing Excel file is not supported by FExcelExporter. Use FExcelAppender or SpreadsheetWriter for append/replace operations.") + } + ``` + +### 3.2 [ ] Replace undeclared `commons-io` transitive dependency in `FOdsImporter` +- **File**: `src/main/groovy/se/alipsa/matrix/spreadsheet/fastods/FOdsImporter.groovy` line 5, 197 +- **Issue**: `IOUtils.toByteArray(is)` is used via `org.apache.commons.io.IOUtils`, which is only available as a transitive dependency from `fast.excel.reader`. It is not declared in `build.gradle`. If the transitive chain changes this dependency will break silently. +- **Fix**: Replace with Groovy's built-in `is.bytes` and remove the import: + ```groovy + // Before + import org.apache.commons.io.IOUtils + byte[] bytes = IOUtils.toByteArray(is) + // After + byte[] bytes = is.bytes + ``` + +### 3.3 [ ] Make `SpreadsheetImporter.validateNotNull` private +- **File**: `src/main/groovy/se/alipsa/matrix/spreadsheet/SpreadsheetImporter.groovy` line 291 +- **Issue**: `static void validateNotNull(Object paramVal, String paramName)` has default (public) Groovy visibility and is therefore part of the public API surface, which is unintentional. +- **Fix**: Add `private` modifier. + +### 3.4 [ ] Add `@CompileStatic` to `FExcelExporter` or document why it is absent +- **File**: `src/main/groovy/se/alipsa/matrix/spreadsheet/fastexcel/FExcelExporter.groovy` lines 21–22 +- **Issue**: `@CompileStatic` is commented out with only a brief note about a module access error on `GenericStyleSetter`. The entire exporter runs with dynamic dispatch as a result. +- **Fix**: Investigate whether the access error is avoidable (e.g. `--add-opens` JVM arg, or avoiding the offending call). If unavoidable, expand the comment to explain the root cause and link to the upstream fastexcel issue so the limitation is tracked rather than forgotten. + +### 3.5 [ ] Add class-level GroovyDoc to `SpreadsheetImporter` +- **File**: `src/main/groovy/se/alipsa/matrix/spreadsheet/SpreadsheetImporter.groovy` +- **Issue**: `SpreadsheetImporter` is the primary import facade but has no class-level documentation. `SpreadsheetWriter` has thorough class doc including a usage example; `SpreadsheetImporter` should follow the same pattern. + +### 3.6 [ ] Fix `@param` mismatches in `SpreadsheetImporter.importOds` / `importExcel` +- **File**: `src/main/groovy/se/alipsa/matrix/spreadsheet/SpreadsheetImporter.groovy` lines 41–123 +- **Issue**: The `importOds(URL url, ...)` and `importExcel(URL url, ...)` overloads copy-paste the doc block from the file-path variants and say `@param file the ods or excel file to import`, but the parameter is `url`, not `file`. Several overloads also have no GroovyDoc at all. +- **Fix**: Correct the `@param` tag to `@param url` with an appropriate description; add missing GroovyDoc to undocumented overloads. + +### 3.7 [ ] Add GroovyDoc to `SpreadsheetWriter.writeSheets(Map params)` +- **File**: `src/main/groovy/se/alipsa/matrix/spreadsheet/SpreadsheetWriter.groovy` line 210 +- **Issue**: The map-params overload is undocumented — which keys are valid, what types, which are required? +- **Fix**: Add a `@param params` block listing `file` (File), `data` (List<Matrix>), and `sheetNames` (List<String>), matching the style used for the other overloads. + +### 3.8 [ ] Standardise `startCol`/`endCol` parameter naming across `Importer` and `SpreadsheetImporter` +- **Files**: `src/main/groovy/se/alipsa/matrix/spreadsheet/Importer.groovy`, `SpreadsheetImporter.groovy` +- **Issue**: Some overloads use `startColumn`/`endColumn` (e.g. `importSpreadsheet(String, int, int, int, String, String, boolean)`) while the majority use `startCol`/`endCol`. The inconsistency appears in both the interface and the facade. +- **Fix**: Standardise on `startCol`/`endCol` throughout. + +### 3.9 [ ] Fix misleading GroovyDoc on `FExcelExporter.exportExcel` overloads +- **File**: `src/main/groovy/se/alipsa/matrix/spreadsheet/fastexcel/FExcelExporter.groovy` lines 29–78 +- **Issue**: All four `exportExcel` overloads claim "if the excel file exists and is not empty, a new sheet will be added to the excel". Since v2.3.0 the method unconditionally throws `IllegalArgumentException` when the file already exists. The GroovyDoc is stale and misleading. +- **Fix**: Update the GroovyDoc to state that existing files cause an `IllegalArgumentException` and direct users to `SpreadsheetWriter` or `FExcelAppender` for append/replace behavior. + +### 3.10 [ ] Add `@CompileStatic` to `ValueExtractor` +- **File**: `src/main/groovy/se/alipsa/matrix/spreadsheet/ValueExtractor.groovy` +- **Issue**: The abstract utility class lacks `@CompileStatic`, running with dynamic dispatch despite being pure type-conversion logic. This is inconsistent with the project's static-compilation preference. +- **Fix**: Add `@CompileStatic` and resolve any typing issues that surface. + +### 3.11 [ ] Tighten `Sheet.name` field type in `fastods` +- **File**: `src/main/groovy/se/alipsa/matrix/spreadsheet/fastods/Sheet.groovy` +- **Issue**: The `name` field and `getName()` return type are `Object`, but the class only ever stores `String` or `Integer` (via the typed `setName` overloads). Using `Object` hides the actual domain from callers and skips IDE assistance. +- **Fix**: Store name as `String` internally ( callers can pass `Integer` and it is converted immediately in `setName(Number)` ), or split into `sheetName` / `sheetNumber` fields. + +### 3.12 [ ] Improve error messages when a named sheet does not exist +- **Files**: `src/main/groovy/se/alipsa/matrix/spreadsheet/fastexcel/FExcelImporter.groovy` lines 160, 172; `fastods/FOdsImporter.groovy` equivalent paths +- **Issue**: `workbook.findSheet(sheetName).orElseThrow()` and `odsDataReader.readOds(..., sheetName, ...)` produce generic `NoSuchElementException: No value present` (or null-sheet errors) instead of naming the missing sheet. This compounds the `'Sheet1'` default problem (4.3) by giving users no clue which sheet name was sought. +- **Fix**: Supply explicit error messages such as `"Sheet '${sheetName}' does not exist in the workbook"`. + +--- + +## Priority 4: Usability Improvements + +### 4.1 [ ] Add `File`-based overloads to `SpreadsheetImporter` +- **Files**: `src/main/groovy/se/alipsa/matrix/spreadsheet/SpreadsheetImporter.groovy`, `Importer.groovy` +- **Issue**: `SpreadsheetWriter.write` accepts `File` as its primary target type, but every `SpreadsheetImporter.importSpreadsheet` overload takes `String` file paths. Users with a `File` object must call `.absolutePath` at every call site. +- **Fix**: Add `File`-accepting overloads that delegate via `file.absolutePath`, matching the writer's API shape. + +### 4.2 [ ] Add whole-sheet convenience import (auto-detect dimensions) +- **File**: `src/main/groovy/se/alipsa/matrix/spreadsheet/SpreadsheetImporter.groovy` +- **Issue**: Every import method requires explicit `endRow` and `endCol`. The `SpreadsheetFormatProvider` already auto-detects these via `SpreadsheetReader.findLastRow`/`findLastCol`, but this is not exposed through `SpreadsheetImporter`. Importing a whole sheet currently requires 3–4 extra method calls (open reader, find last row, find last col, close, then import). +- **Fix**: Add convenience overloads that use `SpreadsheetReader` internally to detect dimensions: + ```groovy + static Matrix importSpreadsheet(File file, int sheetNumber = 1, boolean firstRowAsColNames = true) + static Matrix importSpreadsheet(String file, int sheetNumber = 1, boolean firstRowAsColNames = true) + static Matrix importSpreadsheet(String file, String sheetName, boolean firstRowAsColNames = true) + ``` + +### 4.3 [ ] Fix `'Sheet1'` hardcoded default in static overloads +- **File**: `src/main/groovy/se/alipsa/matrix/spreadsheet/SpreadsheetImporter.groovy` lines 161, 185 +- **Issue**: `importSpreadsheet(String, String sheet = 'Sheet1', ...)` defaults to the literal string `"Sheet1"`. The map-based overload (line 225) correctly defaults to sheet index `1`. Files without a sheet named "Sheet1" silently fail via the static overloads. The v1.2.0 release notes say "Default to 1st sheet instead of 'Sheet1'" but only the map API was updated. +- **Fix**: Change the string default to a numeric sheet-index default (using the auto-detect approach from 4.2), or at minimum document the limitation clearly in GroovyDoc. + +### 4.4 [ ] Refresh README dependency versions and remove stale Log4j claim +- **File**: `matrix-spreadsheet/README.md` +- **Issue**: The README references outdated versions (`matrix-core:3.6.0`, `groovy:5.0.4`, `matrix-spreadsheet:2.3.0`). It also lists **Log4j** as a third-party library (line 248), but the module migrated to `matrix-core` Logger in v2.3.0 and has no direct Log4j dependency. +- **Fix**: Update version references to current aligned versions and remove the Log4j section. + +--- + +## Priority 5: Release Checklist + +- [ ] Remove SNAPSHOT suffix from build.gradle version when cutting release +- [ ] Update `release.md` v2.4.0 notes with current test count (124 tests) and completed fixes diff --git a/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/FileUtil.groovy b/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/FileUtil.groovy index e9c0af579..16ff95a64 100644 --- a/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/FileUtil.groovy +++ b/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/FileUtil.groovy @@ -18,21 +18,21 @@ class FileUtil { * Verify that the filePath exists and is reachable * @param filePath the path + file name of the resource to find * @return a File of found - * @throws Exception if the filePath cannot be found + * @throws SpreadsheetImportException if the filePath cannot be found */ - static File checkFilePath(String filePath) throws Exception { + static File checkFilePath(String filePath) throws SpreadsheetImportException { File excelFile URL url = getResourceUrl(filePath) if (url == null) { - throw new Exception(filePath + " does not exist") + throw new SpreadsheetImportException(filePath + " does not exist") } try { excelFile = Paths.get(url.toURI()).toFile() } catch (URISyntaxException | RuntimeException e) { - throw new Exception(filePath + " does not exist", e) + throw new SpreadsheetImportException(filePath + " does not exist", e) } if (!excelFile.exists()) { - throw new Exception(filePath + " does not exist") + throw new SpreadsheetImportException(filePath + " does not exist") } return excelFile } diff --git a/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/Importer.groovy b/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/Importer.groovy index 0993d68bc..f2019848c 100644 --- a/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/Importer.groovy +++ b/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/Importer.groovy @@ -55,7 +55,7 @@ interface Importer { Matrix importSpreadsheet(String file, int sheetNumber, int startRow, int endRow, - String startColumn, String endColumn, + String startCol, String endCol, boolean firstRowAsColNames) Matrix importSpreadsheet(String file, String sheetName, diff --git a/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/SpreadsheetImporter.groovy b/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/SpreadsheetImporter.groovy index 1da7ef01e..5edeaad30 100644 --- a/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/SpreadsheetImporter.groovy +++ b/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/SpreadsheetImporter.groovy @@ -12,153 +12,182 @@ import java.text.NumberFormat class SpreadsheetImporter { /** + * Static facade for importing spreadsheet data into Matrix objects. + * + *

Auto-detects the file format (.ods vs .xlsx) and delegates to the + * appropriate importer ({@link FOdsImporter} or {@link FExcelImporter}).

+ * + *

Basic Usage

+ * 
+   * // Import by sheet index (1-based)
+   * Matrix data = SpreadsheetImporter.importSpreadsheet("data.xlsx", 1, 1, 100, 1, 5)
+   *
+   * // Import by sheet name
+   * Matrix data = SpreadsheetImporter.importSpreadsheet("data.xlsx", "Sales", 1, 100, 1, 5)
+   *
+   * // Import using named parameters
+   * Matrix data = SpreadsheetImporter.importSpreadsheet(
+   *   file: "data.xlsx", sheet: 1, endRow: 100, endCol: 5)
+   *
+ * + * @see SpreadsheetWriter + * @see FExcelImporter + * @see FOdsImporter + */ + + /** + * Import a spreadsheet by file path and sheet index. * * @param file the ods or excel file to import * @param sheet the sheet index (index starting with 1) * @param startRow the first row to include (the first row is row 1) * @param endRow the last row to include - * @param startColumn the first column to include (index starting with 1) - * @param endColumn the last column to include + * @param startCol the first column to include (index starting with 1) + * @param endCol the last column to include * @param firstRowAsColNames whether to treat the first row as a header row for the column names or not, * defaults to true * @return A Matrix corresponding to the spreadsheet data. */ static Matrix importSpreadsheet(String file, int sheet, int startRow = 1, int endRow, - int startColumn = 1, int endColumn, + int startCol = 1, int endCol, boolean firstRowAsColNames = true) { if (isOdsFile(file)) { - return odsImporter().importSpreadsheet(file, sheet, startRow, endRow, startColumn, endColumn, firstRowAsColNames) + return odsImporter().importSpreadsheet(file, sheet, startRow, endRow, startCol, endCol, firstRowAsColNames) } SpreadsheetUtil.ensureXlsx(file) return excelImporter() - .importSpreadsheet(file, sheet, startRow, endRow, startColumn, endColumn, firstRowAsColNames) + .importSpreadsheet(file, sheet, startRow, endRow, startCol, endCol, firstRowAsColNames) } /** + * Import an ODS spreadsheet from a URL by sheet index. * - * @param file the ods or excel file to import + * @param url the URL of the ods file to import * @param sheet the sheet index (index starting with 1) * @param startRow the first row to include (the first row is row 1) * @param endRow the last row to include - * @param startColumn the first column to include (index starting with 1) - * @param endColumn the last column to include + * @param startCol the first column to include (index starting with 1) + * @param endCol the last column to include * @param firstRowAsColNames whether to treat the first row as a header row for the column names or not, * defaults to true * @return A Matrix corresponding to the spreadsheet data. */ static Matrix importOds(URL url, int sheet, int startRow = 1, int endRow, - int startColumn = 1, int endColumn, + int startCol = 1, int endCol, boolean firstRowAsColNames = true) { - odsImporter().importSpreadsheet(url, sheet, startRow, endRow, startColumn, endColumn, firstRowAsColNames) + odsImporter().importSpreadsheet(url, sheet, startRow, endRow, startCol, endCol, firstRowAsColNames) } static Matrix importOds(URL url, int sheet, int startRow = 1, int endRow, - String startColumn = 'A', String endColumn, + String startCol = 'A', String endCol, boolean firstRowAsColNames = true) { - odsImporter().importSpreadsheet(url, sheet, startRow, endRow, startColumn, endColumn, firstRowAsColNames) + odsImporter().importSpreadsheet(url, sheet, startRow, endRow, startCol, endCol, firstRowAsColNames) } /** + * Import an ODS spreadsheet from a URL by sheet name. * - * @param file the ods or excel file to import - * @param sheet the sheet name, defaults to Sheet1 + * @param url the URL of the ods file to import + * @param sheet the sheet name * @param startRow the first row to include (the first row is row 1) * @param endRow the last row to include - * @param startColumn the first column to include (index starting with 1) - * @param endColumn the last column to include + * @param startCol the first column to include (index starting with 1) + * @param endCol the last column to include * @param firstRowAsColNames whether to treat the first row as a header row for the column names or not, * defaults to true * @return A Matrix corresponding to the spreadsheet data. */ static Matrix importOds(URL url, String sheet, int startRow = 1, int endRow, - int startColumn = 1, int endColumn, + int startCol = 1, int endCol, boolean firstRowAsColNames = true) { - odsImporter().importSpreadsheet(url, sheet, startRow, endRow, startColumn, endColumn, firstRowAsColNames) + odsImporter().importSpreadsheet(url, sheet, startRow, endRow, startCol, endCol, firstRowAsColNames) } static Matrix importOds(URL url, String sheet, int startRow = 1, int endRow, - String startColumn = 'A', String endColumn, + String startCol = 'A', String endCol, boolean firstRowAsColNames = true) { - odsImporter().importSpreadsheet(url, sheet, startRow, endRow, startColumn, endColumn, firstRowAsColNames) + odsImporter().importSpreadsheet(url, sheet, startRow, endRow, startCol, endCol, firstRowAsColNames) } static Matrix importExcel(URL url, int sheet, int startRow = 1, int endRow, - int startColumn = 1, int endColumn, + int startCol = 1, int endCol, boolean firstRowAsColNames = true) { SpreadsheetUtil.ensureXlsx(url?.path) - excelImporter().importSpreadsheet(url, sheet, startRow, endRow, startColumn, endColumn, firstRowAsColNames) + excelImporter().importSpreadsheet(url, sheet, startRow, endRow, startCol, endCol, firstRowAsColNames) } static Matrix importExcel(URL url, String sheet, int startRow = 1, int endRow, - int startColumn = 1, int endColumn, + int startCol = 1, int endCol, boolean firstRowAsColNames = true) { SpreadsheetUtil.ensureXlsx(url?.path) - excelImporter().importSpreadsheet(url, sheet, startRow, endRow, startColumn, endColumn, firstRowAsColNames) + excelImporter().importSpreadsheet(url, sheet, startRow, endRow, startCol, endCol, firstRowAsColNames) } static Matrix importExcel(URL url, int sheet, int startRow = 1, int endRow, - String startColumn = 'A', String endColumn, + String startCol = 'A', String endCol, boolean firstRowAsColNames = true) { SpreadsheetUtil.ensureXlsx(url?.path) - excelImporter().importSpreadsheet(url, sheet, startRow, endRow, startColumn, endColumn, firstRowAsColNames) + excelImporter().importSpreadsheet(url, sheet, startRow, endRow, startCol, endCol, firstRowAsColNames) } static Matrix importExcel(URL url, String sheet, int startRow = 1, int endRow, - String startColumn = 'A', String endColumn, + String startCol = 'A', String endCol, boolean firstRowAsColNames = true) { SpreadsheetUtil.ensureXlsx(url?.path) - excelImporter().importSpreadsheet(url, sheet, startRow, endRow, startColumn, endColumn, firstRowAsColNames) + excelImporter().importSpreadsheet(url, sheet, startRow, endRow, startCol, endCol, firstRowAsColNames) } /** + * Import a spreadsheet by file path and sheet index using column names. * * @param file the ods or excel file to import * @param sheet the sheet index (index starting with 1) * @param startRow the first row to include (the first row is row 1) * @param endRow the last row to include - * @param startColumn the first column name to include (A is the first column name etc.) - * @param endColumn the last column name to include - * @param firstRowAsColNames whether to treat the first row as a header row for thecolumn names or not, + * @param startCol the first column name to include (A is the first column name etc.) + * @param endCol the last column name to include + * @param firstRowAsColNames whether to treat the first row as a header row for the column names or not, * defaults to true * @return A Matrix corresponding to the spreadsheet data. */ static Matrix importSpreadsheet(String file, int sheet, int startRow = 1, int endRow, - String startColumn = 'A', String endColumn, + String startCol = 'A', String endCol, boolean firstRowAsColNames = true) { if (isOdsFile(file)) { - return odsImporter().importSpreadsheet(file, sheet, startRow, endRow, startColumn, endColumn, firstRowAsColNames) + return odsImporter().importSpreadsheet(file, sheet, startRow, endRow, startCol, endCol, firstRowAsColNames) } SpreadsheetUtil.ensureXlsx(file) return excelImporter() - .importSpreadsheet(file, sheet, startRow, endRow, startColumn, endColumn, firstRowAsColNames) + .importSpreadsheet(file, sheet, startRow, endRow, startCol, endCol, firstRowAsColNames) } /** + * Import a spreadsheet by file path and sheet name. * * @param file the ods or excel file to import - * @param sheet the sheet name, defaults to Sheet1 + * @param sheet the sheet name * @param startRow the first row to include (the first row is row 1) * @param endRow the last row to include - * @param startColumn the first column to include (index starting with 1) - * @param endColumn the last column to include - * @param firstRowAsColNames whether to treat the first row as a header row for thecolumn names or not, + * @param startCol the first column to include (index starting with 1) + * @param endCol the last column to include + * @param firstRowAsColNames whether to treat the first row as a header row for the column names or not, * defaults to true * @return A Matrix corresponding to the spreadsheet data. */ - static Matrix importSpreadsheet(String file, String sheet = 'Sheet1', + static Matrix importSpreadsheet(String file, String sheet, int startRow = 1, int endRow, int startCol = 1, int endCol, boolean firstRowAsColNames = true) { @@ -171,18 +200,19 @@ class SpreadsheetImporter { } /** + * Import a spreadsheet by file path and sheet name using column names. * * @param file the ods or excel file to import - * @param sheet the sheet name, defaults to Sheet1 + * @param sheet the sheet name * @param startRow the first row to include (the first row is row 1) * @param endRow the last row to include - * @param startColumn the first column name to include (A is the first column name etc.) - * @param endColumn the last column name to include - * @param firstRowAsColNames whether to treat the first row as a header row for thecolumn names or not, + * @param startCol the first column name to include (A is the first column name etc.) + * @param endCol the last column name to include + * @param firstRowAsColNames whether to treat the first row as a header row for the column names or not, * defaults to true * @return A Matrix corresponding to the spreadsheet data. */ - static Matrix importSpreadsheet(String file, String sheet = 'Sheet1', + static Matrix importSpreadsheet(String file, String sheet, int startRow = 1, int endRow, String startCol = 'A', String endCol, boolean firstRowAsColNames = true) { @@ -209,6 +239,90 @@ class SpreadsheetImporter { * firstRowAsColNames (a boolean) * @return A Matrix corresponding to the spreadsheet data. */ + // --- File-based overloads --- + + static Matrix importSpreadsheet(File file, int sheet, + int startRow = 1, int endRow, + int startCol = 1, int endCol, + boolean firstRowAsColNames = true) { + importSpreadsheet(file.absolutePath, sheet, startRow, endRow, startCol, endCol, firstRowAsColNames) + } + + static Matrix importSpreadsheet(File file, String sheet, + int startRow = 1, int endRow, + int startCol = 1, int endCol, + boolean firstRowAsColNames = true) { + importSpreadsheet(file.absolutePath, sheet, startRow, endRow, startCol, endCol, firstRowAsColNames) + } + + static Matrix importSpreadsheet(File file, int sheet, + int startRow = 1, int endRow, + String startCol = 'A', String endCol, + boolean firstRowAsColNames = true) { + importSpreadsheet(file.absolutePath, sheet, startRow, endRow, startCol, endCol, firstRowAsColNames) + } + + static Matrix importSpreadsheet(File file, String sheet, + int startRow = 1, int endRow, + String startCol = 'A', String endCol, + boolean firstRowAsColNames = true) { + importSpreadsheet(file.absolutePath, sheet, startRow, endRow, startCol, endCol, firstRowAsColNames) + } + + // --- Whole-sheet convenience imports (auto-detect dimensions) --- + + /** + * Import an entire spreadsheet sheet using auto-detected dimensions. + * + * @param file the spreadsheet file to import + * @param sheetNumber the sheet index (1-based), defaults to 1 + * @param firstRowAsColNames whether to treat the first row as column names + * @return A Matrix corresponding to the spreadsheet data. + */ + static Matrix importSpreadsheet(File file, int sheetNumber = 1, boolean firstRowAsColNames = true) { + SpreadsheetReader.Factory.create(file).withCloseable { SpreadsheetReader reader -> + int endRow = reader.findLastRow(sheetNumber) + int endCol = reader.findLastCol(sheetNumber) + return importSpreadsheet(file.absolutePath, sheetNumber, 1, endRow, 1, endCol, firstRowAsColNames) + } + } + + /** + * Import an entire spreadsheet sheet using auto-detected dimensions. + * + * @param file the spreadsheet file path to import + * @param sheetNumber the sheet index (1-based), defaults to 1 + * @param firstRowAsColNames whether to treat the first row as column names + * @return A Matrix corresponding to the spreadsheet data. + */ + static Matrix importSpreadsheet(String file, int sheetNumber = 1, boolean firstRowAsColNames = true) { + File spreadsheetFile = FileUtil.checkFilePath(file) + SpreadsheetReader.Factory.create(spreadsheetFile).withCloseable { SpreadsheetReader reader -> + int endRow = reader.findLastRow(sheetNumber) + int endCol = reader.findLastCol(sheetNumber) + return importSpreadsheet(file, sheetNumber, 1, endRow, 1, endCol, firstRowAsColNames) + } + } + + /** + * Import an entire spreadsheet sheet by name using auto-detected dimensions. + * + * @param file the spreadsheet file path to import + * @param sheetName the sheet name + * @param firstRowAsColNames whether to treat the first row as column names + * @return A Matrix corresponding to the spreadsheet data. + */ + static Matrix importSpreadsheet(String file, String sheetName, boolean firstRowAsColNames = true) { + File spreadsheetFile = FileUtil.checkFilePath(file) + SpreadsheetReader.Factory.create(spreadsheetFile).withCloseable { SpreadsheetReader reader -> + int endRow = reader.findLastRow(sheetName) + int endCol = reader.findLastCol(sheetName) + return importSpreadsheet(file, sheetName, 1, endRow, 1, endCol, firstRowAsColNames) + } + } + + // --- Map-based import --- + static Matrix importSpreadsheet(Map params) { def fp = params['file'] validateNotNull(fp, 'file') @@ -263,6 +377,12 @@ class SpreadsheetImporter { ) } + static Map importSpreadsheets(File file, + List sheetParams, + NumberFormat... formatOpt) { + importSpreadsheets(file.absolutePath, sheetParams, formatOpt) + } + static Map importSpreadsheets(String fileName, List sheetParams, NumberFormat... formatOpt) { @@ -288,7 +408,7 @@ class SpreadsheetImporter { .importSpreadsheets(url, sheetParams, formatOpt) } - static void validateNotNull(Object paramVal, String paramName) { + private static void validateNotNull(Object paramVal, String paramName) { if (paramVal == null) { throw new IllegalArgumentException("$paramName cannot be null") } diff --git a/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/SpreadsheetWriter.groovy b/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/SpreadsheetWriter.groovy index 92fa9070e..289e65e44 100644 --- a/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/SpreadsheetWriter.groovy +++ b/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/SpreadsheetWriter.groovy @@ -204,7 +204,14 @@ class SpreadsheetWriter { /** * Write multiple Matrix objects using a parameter map. * - * @param params map with keys: 'file' (File), 'data' (List<Matrix>), 'sheetNames' (List<String>) + *

Valid keys in the params map:

+ *
    + *
  • {@code file} ({@link File}) — the target spreadsheet file (required)
    • + *
  • {@code data} ({@code List}) — the matrices to write, one per sheet (required)
    • + *
  • {@code sheetNames} ({@code List}) — names for each sheet (required)
    • + *
+ * + * @param params map of write parameters * @return list of actual sheet names created */ static List writeSheets(Map params) { diff --git a/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/ValueExtractor.groovy b/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/ValueExtractor.groovy index 12d61ac04..595aa8fd9 100644 --- a/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/ValueExtractor.groovy +++ b/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/ValueExtractor.groovy @@ -1,8 +1,11 @@ package se.alipsa.matrix.spreadsheet +import groovy.transform.CompileStatic + /** * A ValueExtractor is a helper class that makes it easier to get values from a spreadsheet. */ +@CompileStatic abstract class ValueExtractor { Double getDouble(Object val) { diff --git a/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/fastexcel/FExcelExporter.groovy b/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/fastexcel/FExcelExporter.groovy index 43d74c284..a406ccd9b 100644 --- a/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/fastexcel/FExcelExporter.groovy +++ b/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/fastexcel/FExcelExporter.groovy @@ -4,7 +4,6 @@ import groovy.transform.CompileStatic import org.dhatim.fastexcel.Workbook import org.dhatim.fastexcel.Worksheet -import org.dhatim.fastexcel.reader.ReadableWorkbook import se.alipsa.matrix.core.Column import se.alipsa.matrix.core.Matrix @@ -17,9 +16,12 @@ import java.time.LocalDateTime import java.time.ZonedDateTime import java.util.Collections -// Causes module access error (failed to access class org.dhatim.fastexcel.GenericStyleSetter) -// so go with dynamic for now -//@CompileStatic +// @CompileStatic is intentionally omitted. +// When enabled, Groovy generates direct bytecode for sheet.style(row, col).format(...).set() +// which triggers an IllegalAccessError at runtime because GenericStyleSetter is +// package-private inside org.dhatim.fastexcel. Dynamic dispatch uses reflection/ +// metaclass invocation which bypasses this restriction. Re-evaluate if fastexcel +// ever makes GenericStyleSetter public. class FExcelExporter { static final Logger logger = Logger.getLogger(FExcelExporter) @@ -27,9 +29,12 @@ class FExcelExporter { static final String VERSION = '02.1000' // must be in the format XX.YYYY /** - * Export to an Excel file. If the file does not exists, a new file will be created - * if the excel file exists and is not empty, a new sheet will be added to the excel - * The name of the sheet will correspond to the name of the Matrix + * Export a Matrix to a new Excel file. + * + *

If the file already exists and is not empty, an {@link IllegalArgumentException} + * is thrown. Use {@link FExcelAppender} or {@link SpreadsheetWriter} for append/replace + * operations.

+ * * @param filePath the path to the file to export * @param data the Matrix data to export * @return the actual name of the sheet created (illegal characters replaced by space) @@ -40,9 +45,12 @@ class FExcelExporter { } /** - * Export to an Excel file. If the file does not exists, a new file will be created - * if the excel file exists and is not empty, a new sheet will be added to the excel - * The name of the sheet will correspond to the name of the Matrix + * Export a Matrix to a new Excel file. + * + *

If the file already exists and is not empty, an {@link IllegalArgumentException} + * is thrown. Use {@link FExcelAppender} or {@link SpreadsheetWriter} for append/replace + * operations.

+ * * @param file the file to export * @param data the Matrix data to export * @return the actual name of the sheet created (illegal characters replaced by space) @@ -55,8 +63,11 @@ class FExcelExporter { } /** - * Export to an Excel file. If the file does not exists, a new file will be created - * if the excel file exists and is not empty, a new sheet will be added to the excel + * Export a Matrix to a new Excel file with a specific sheet name. + * + *

If the file already exists and is not empty, an {@link IllegalArgumentException} + * is thrown. Use {@link FExcelAppender} or {@link SpreadsheetWriter} for append/replace + * operations.

* * @param file the file to export * @param data the Matrix data to export @@ -68,7 +79,11 @@ class FExcelExporter { } /** - * Export to an Excel file with a specific start position. + * Export a Matrix to a new Excel file with a specific start position. + * + *

If the file already exists and is not empty, an {@link IllegalArgumentException} + * is thrown. Use {@link FExcelAppender} or {@link SpreadsheetWriter} for append/replace + * operations.

* * @param file the file to export * @param data the Matrix data to export @@ -81,26 +96,12 @@ class FExcelExporter { String validSheetName = SpreadsheetUtil.createValidSheetName(sheetName) SpreadsheetUtil.CellPosition position = SpreadsheetUtil.parseCellPosition(startPosition) if (file.exists() && file.length() > 0) { - try (FileInputStream fis = new FileInputStream(file) - ReadableWorkbook workbook = new ReadableWorkbook(fis, FExcelImporter.OPTIONS)) { - List sheetNames = FExcelReader.getSheetNames(workbook) - if (sheetNames.contains(validSheetName)) { - int index = 1 - while (true) { - validSheetName = SpreadsheetUtil.createValidSheetName(validSheetName + index++) - if (!sheetNames.contains(validSheetName)) { - break - } - } - } - throw new IllegalArgumentException("Appending to an existing Excel file is not supported by FExcelExporter. Use FExcelAppender or SpreadsheetWriter for append/replace operations.") - } - } else { - try (FileOutputStream fos = new FileOutputStream(file); Workbook workbook = new Workbook(fos, APP_NAME, VERSION)) { - Worksheet sheet = workbook.newWorksheet(validSheetName) - buildSheet(data, sheet, position) - workbook.finish() - } + throw new IllegalArgumentException("Appending to an existing Excel file is not supported by FExcelExporter. Use FExcelAppender or SpreadsheetWriter for append/replace operations.") + } + try (FileOutputStream fos = new FileOutputStream(file); Workbook workbook = new Workbook(fos, APP_NAME, VERSION)) { + Worksheet sheet = workbook.newWorksheet(validSheetName) + buildSheet(data, sheet, position) + workbook.finish() } return validSheetName } diff --git a/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/fastexcel/FExcelImporter.groovy b/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/fastexcel/FExcelImporter.groovy index 1d09d4f79..63beed9bb 100644 --- a/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/fastexcel/FExcelImporter.groovy +++ b/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/fastexcel/FExcelImporter.groovy @@ -135,7 +135,7 @@ class FExcelImporter implements Importer { @Override Matrix importSpreadsheet(String file, int sheet, int startRow = 1, int endRow, - String startColumn = 'A', String endColumn, + String startCol = 'A', String endCol, boolean firstRowAsColNames = true) { SpreadsheetUtil.ensureXlsx(file) return importSpreadsheet( @@ -143,8 +143,8 @@ class FExcelImporter implements Importer { sheet, startRow, endRow, - SpreadsheetUtil.asColumnNumber(startColumn), - SpreadsheetUtil.asColumnNumber(endColumn), + SpreadsheetUtil.asColumnNumber(startCol), + SpreadsheetUtil.asColumnNumber(endCol), firstRowAsColNames ) } @@ -157,7 +157,9 @@ class FExcelImporter implements Importer { SpreadsheetUtil.ensureXlsx(file) File excelFile = FileUtil.checkFilePath(file) try (ReadableWorkbook workbook = new ReadableWorkbook(excelFile, OPTIONS)) { - Sheet sheet = workbook.findSheet(sheetName).orElseThrow() + Sheet sheet = workbook.findSheet(sheetName).orElseThrow { + new NoSuchElementException("Sheet '${sheetName}' does not exist in the workbook") + } boolean isDate1904 = workbook.isDate1904() return importExcelSheet(sheet, startRow, endRow, startCol, endCol, firstRowAsColNames, isDate1904) } @@ -169,7 +171,9 @@ class FExcelImporter implements Importer { int startCol = 1, int endCol, boolean firstRowAsColNames = true) { try (ReadableWorkbook workbook = new ReadableWorkbook(is, OPTIONS)) { - Sheet sheet = workbook.findSheet(sheetName).orElseThrow() + Sheet sheet = workbook.findSheet(sheetName).orElseThrow { + new NoSuchElementException("Sheet '${sheetName}' does not exist in the workbook") + } boolean isDate1904 = workbook.isDate1904() def m = importExcelSheet(sheet, startRow, endRow, startCol, endCol, firstRowAsColNames, isDate1904) return m diff --git a/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/fastods/FOdsImporter.groovy b/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/fastods/FOdsImporter.groovy index 1977cb1c0..17150d4b1 100644 --- a/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/fastods/FOdsImporter.groovy +++ b/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/fastods/FOdsImporter.groovy @@ -2,8 +2,6 @@ package se.alipsa.matrix.spreadsheet.fastods import groovy.transform.CompileStatic -import org.apache.commons.io.IOUtils - import se.alipsa.matrix.core.ListConverter import se.alipsa.matrix.core.Matrix import se.alipsa.matrix.core.ValueConverter @@ -74,6 +72,9 @@ class FOdsImporter implements Importer { sheet = odsDataReader.readOds(fis, sheetName, startRow, endRow, startCol, endCol) } + if (sheet == null) { + throw new IllegalArgumentException("Sheet '${sheetName}' does not exist in the workbook") + } return buildMatrix(sheet, firstRowAsColNames) } @@ -111,6 +112,9 @@ class FOdsImporter implements Importer { int startCol = 1, int endCol, boolean firstRowAsColNames = true) { def sheet = odsDataReader.readOds(is, sheetName, startRow, endRow, startCol, endCol) + if (sheet == null) { + throw new IllegalArgumentException("Sheet '${sheetName}' does not exist in the workbook") + } return buildMatrix(sheet, firstRowAsColNames) } @@ -194,7 +198,7 @@ class FOdsImporter implements Importer { @Override Map importSpreadsheets(InputStream is, List sheetParams, NumberFormat... formatOpt) { - byte[] bytes = IOUtils.toByteArray(is) + byte[] bytes = is.bytes Map result = [:] sheetParams.each { try (InputStream is2 = new ByteArrayInputStream(bytes)) { diff --git a/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/fastods/Sheet.groovy b/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/fastods/Sheet.groovy index b115908a3..4c058f783 100644 --- a/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/fastods/Sheet.groovy +++ b/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/fastods/Sheet.groovy @@ -5,14 +5,14 @@ import groovy.transform.CompileStatic @CompileStatic class Sheet extends ArrayList> { - Object name + String name - Object getName() { + String getName() { name } String getSheetName() { - String.valueOf(name) + name } void setName(String name) { @@ -20,10 +20,10 @@ class Sheet extends ArrayList> { } void setName(Integer name) { - this.name = name + this.name = String.valueOf(name) } void setName(Number name) { - this.name = name.intValue() + this.name = String.valueOf(name.intValue()) } } diff --git a/matrix-spreadsheet/v2.4.0-roadmap.md b/matrix-spreadsheet/v2.4.0-roadmap.md deleted file mode 100644 index 793ad097b..000000000 --- a/matrix-spreadsheet/v2.4.0-roadmap.md +++ /dev/null @@ -1,52 +0,0 @@ -# Matrix Spreadsheet v2.4.0 Release Roadmap - -This document outlines the planned work for matrix-spreadsheet v2.4.0. - -## Summary - -v2.4.0 focuses on targeted refactoring, stronger test coverage (>= 70%), -performance benchmarking, and release hygiene. - ---- - -## Priority 1: Testing & Performance (Must Do) - -### 1.1 [x] Test coverage plan (target >= 70%) -- ✅ Completed in v2.3.0: Coverage achieved 79.74% (exceeds 70% target) -- See the detailed plan in v2.3.0 roadmap under "Test Coverage & Performance Plan". - -### 1.2 [x] Add performance benchmarks for XLSX/ODS read/write -- ✅ Completed in v2.3.0: SpreadsheetBenchmark added with 50k x 12 and Crime_Data tests -- Baseline read/write timing for realistic datasets documented in v2.3.0 roadmap. - -### 1.3 [x] Fix any performance regressions found in benchmarks -- ✅ Completed in v2.3.0: ODS performance improved 65-79% via Aalto parser and optimizations -- Before/after metrics captured: 4.86s → 1.70s (medium), 262s → 53s (large) - ---- - -## Priority 2: Refactoring (Should Do) - -### 2.1 [ ] Consider splitting FExcelAppender -- **File**: `src/main/groovy/se/alipsa/matrix/spreadsheet/fastexcel/FExcelAppender.groovy` -- **Issue**: ~750 lines with 11 inner classes -- **Consideration**: Could extract `WorkbookState`, `WorkbookPlan`, `SheetTemplate` to separate files -- **Note**: The class is well-structured internally; this is optional cleanup - -### 2.2 [ ] Consider using Optional in FExcelReader -- **File**: `src/main/groovy/se/alipsa/matrix/spreadsheet/fastexcel/FExcelReader.groovy` -- **Issue**: Returns -1 when sheet/content not found (lines 85, 97, 146) -- **Consideration**: Modern Java practice would use Optional -- **Note**: This is a breaking API change; defer to v3.0 if needed - -### 2.3 [ ] Consider more specific exception types in FileUtil -- **File**: `src/main/groovy/se/alipsa/matrix/spreadsheet/FileUtil.groovy` -- **Issue**: Throws generic `Exception` in several places -- **Note**: Low priority; current behavior works - ---- - -## Priority 3: Release Checklist - -- [ ] Remove SNAPSHOT suffix from build.gradle version when cutting release - From b2a573795531d20f297cd30653c6c0bf1aa81e8f Mon Sep 17 00:00:00 2001 From: per Date: Tue, 5 May 2026 23:19:08 +0200 Subject: [PATCH 2/4] Address PR review feedback - Fix misplaced class-level GroovyDoc in SpreadsheetImporter (move before @CompileStatic) - Add tests for new File-based and auto-detect convenience overloads - Remove matrix-tablesaw entries from matrix-spreadsheet release.md - Add one-liner GroovyDoc to File-based delegation overloads - Roadmap file: main has v2.4.0-roadmap.md at module root; PR moves it to req/ subdirectory. No merge conflict since git tracks the move correctly. --- matrix-spreadsheet/release.md | 2 - .../spreadsheet/SpreadsheetImporter.groovy | 63 ++++++++++++------- .../SpreadsheetImporterTest.groovy | 14 +++++ 3 files changed, 53 insertions(+), 26 deletions(-) diff --git a/matrix-spreadsheet/release.md b/matrix-spreadsheet/release.md index 1afee9fe8..2b94873b2 100644 --- a/matrix-spreadsheet/release.md +++ b/matrix-spreadsheet/release.md @@ -26,8 +26,6 @@ - add `@CompileStatic` to `ValueExtractor` - replace generic `Exception` throws in `FileUtil` with `SpreadsheetImportException` - document why `FExcelExporter` cannot use `@CompileStatic` (fastexcel internal `GenericStyleSetter` access) -- fix malformed `@code{...}` inline tags in `Gtable.groovy` GroovyDoc -- replace unbounded generic `` in `TableUtil.createColumn` with concrete `ColumnType` parameter ### Test Coverage - 124 tests passing (up from 105 in v2.3.0) diff --git a/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/SpreadsheetImporter.groovy b/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/SpreadsheetImporter.groovy index 5edeaad30..526ed3085 100644 --- a/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/SpreadsheetImporter.groovy +++ b/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/SpreadsheetImporter.groovy @@ -8,33 +8,32 @@ import se.alipsa.matrix.spreadsheet.fastods.FOdsImporter import java.text.NumberFormat +/** + * Static facade for importing spreadsheet data into Matrix objects. + * + *

Auto-detects the file format (.ods vs .xlsx) and delegates to the + * appropriate importer ({@link FOdsImporter} or {@link FExcelImporter}).

+ * + *

Basic Usage

+ * 
+ * // Import by sheet index (1-based)
+ * Matrix data = SpreadsheetImporter.importSpreadsheet("data.xlsx", 1, 1, 100, 1, 5)
+ *
+ * // Import by sheet name
+ * Matrix data = SpreadsheetImporter.importSpreadsheet("data.xlsx", "Sales", 1, 100, 1, 5)
+ *
+ * // Import using named parameters
+ * Matrix data = SpreadsheetImporter.importSpreadsheet(
+ *   file: "data.xlsx", sheet: 1, endRow: 100, endCol: 5)
+ *
+ * + * @see SpreadsheetWriter + * @see FExcelImporter + * @see FOdsImporter + */ @CompileStatic class SpreadsheetImporter { - /** - * Static facade for importing spreadsheet data into Matrix objects. - * - *

Auto-detects the file format (.ods vs .xlsx) and delegates to the - * appropriate importer ({@link FOdsImporter} or {@link FExcelImporter}).

- * - *

Basic Usage

- * 
-   * // Import by sheet index (1-based)
-   * Matrix data = SpreadsheetImporter.importSpreadsheet("data.xlsx", 1, 1, 100, 1, 5)
-   *
-   * // Import by sheet name
-   * Matrix data = SpreadsheetImporter.importSpreadsheet("data.xlsx", "Sales", 1, 100, 1, 5)
-   *
-   * // Import using named parameters
-   * Matrix data = SpreadsheetImporter.importSpreadsheet(
-   *   file: "data.xlsx", sheet: 1, endRow: 100, endCol: 5)
-   *
- * - * @see SpreadsheetWriter - * @see FExcelImporter - * @see FOdsImporter - */ - /** * Import a spreadsheet by file path and sheet index. * @@ -241,6 +240,10 @@ class SpreadsheetImporter { */ // --- File-based overloads --- + /** + * Delegates to {@link #importSpreadsheet(String, int, int, int, int, int, boolean)} + * after resolving the absolute path. + */ static Matrix importSpreadsheet(File file, int sheet, int startRow = 1, int endRow, int startCol = 1, int endCol, @@ -248,6 +251,10 @@ class SpreadsheetImporter { importSpreadsheet(file.absolutePath, sheet, startRow, endRow, startCol, endCol, firstRowAsColNames) } + /** + * Delegates to {@link #importSpreadsheet(String, String, int, int, int, int, boolean)} + * after resolving the absolute path. + */ static Matrix importSpreadsheet(File file, String sheet, int startRow = 1, int endRow, int startCol = 1, int endCol, @@ -255,6 +262,10 @@ class SpreadsheetImporter { importSpreadsheet(file.absolutePath, sheet, startRow, endRow, startCol, endCol, firstRowAsColNames) } + /** + * Delegates to {@link #importSpreadsheet(String, int, int, int, String, String, boolean)} + * after resolving the absolute path. + */ static Matrix importSpreadsheet(File file, int sheet, int startRow = 1, int endRow, String startCol = 'A', String endCol, @@ -262,6 +273,10 @@ class SpreadsheetImporter { importSpreadsheet(file.absolutePath, sheet, startRow, endRow, startCol, endCol, firstRowAsColNames) } + /** + * Delegates to {@link #importSpreadsheet(String, String, int, int, String, String, boolean)} + * after resolving the absolute path. + */ static Matrix importSpreadsheet(File file, String sheet, int startRow = 1, int endRow, String startCol = 'A', String endCol, diff --git a/matrix-spreadsheet/src/test/groovy/spreadsheet/SpreadsheetImporterTest.groovy b/matrix-spreadsheet/src/test/groovy/spreadsheet/SpreadsheetImporterTest.groovy index 50a450fe5..f62536900 100644 --- a/matrix-spreadsheet/src/test/groovy/spreadsheet/SpreadsheetImporterTest.groovy +++ b/matrix-spreadsheet/src/test/groovy/spreadsheet/SpreadsheetImporterTest.groovy @@ -16,6 +16,7 @@ import se.alipsa.matrix.spreadsheet.Importer import se.alipsa.matrix.spreadsheet.SpreadsheetImporter import se.alipsa.matrix.spreadsheet.fastexcel.FExcelImporter +import java.io.File import java.io.InputStream import java.math.RoundingMode import java.time.LocalDate @@ -199,4 +200,17 @@ class SpreadsheetImporterTest { stream.close() } } + + @Test + void testAutoDetectXlsxByFile() throws Exception { + File file = new File(SpreadsheetImporterTest.class.getResource("/Book1.xlsx").toURI()) + Matrix m = importSpreadsheet(file, 1, true) + book1ImportAssertions(m) + } + + @Test + void testAutoDetectXlsxByName() { + Matrix m = importSpreadsheet("Book1.xlsx", "Sheet1", true) + book1ImportAssertions(m) + } } From 15577c7e04a26a95e39c42ed25b20616cd3ce326 Mon Sep 17 00:00:00 2001 From: pernyf Date: Wed, 6 May 2026 09:22:11 +0200 Subject: [PATCH 3/4] Address PR review findings for spreadsheet v2.4.0 Restore Sheet1 default, add File+sheetName auto-detect overload, fix misplaced GroovyDoc, align exception types in FOdsImporter, use string interpolation in FileUtil, update roadmap checkboxes, and add 7 new tests for coverage gaps. Co-Authored-By: Claude Opus 4.6 --- matrix-spreadsheet/README.md | 2 +- matrix-spreadsheet/release.md | 1 - matrix-spreadsheet/req/v2.4.0-roadmap.md | 44 +++++++------- .../alipsa/matrix/spreadsheet/FileUtil.groovy | 6 +- .../spreadsheet/SpreadsheetImporter.groovy | 44 ++++++++------ .../spreadsheet/fastods/FOdsImporter.groovy | 4 +- .../SpreadsheetImporterTest.groovy | 57 +++++++++++++++++++ 7 files changed, 112 insertions(+), 46 deletions(-) diff --git a/matrix-spreadsheet/README.md b/matrix-spreadsheet/README.md index 5b2f8e9b7..d4402ec5c 100644 --- a/matrix-spreadsheet/README.md +++ b/matrix-spreadsheet/README.md @@ -44,7 +44,7 @@ println(table.head(10)) ``` The SpreadSheetImporter.importSpreadSheetSheet takes the following parameters: - _file_ the filePath or the file object pointing to the Excel file -- _sheetName_ the name of the sheet to import; when omitted, defaults to the first sheet (index 1) +- _sheetName_ the name of the sheet to import, default is 'Sheet1' - _startRow_ the starting row for the import (as you would see the row number in Excel), defaults to 1 - _endRow_ the last row to import - _startCol_ the starting column name (A, B etc.) or column number (1, 2 etc.) diff --git a/matrix-spreadsheet/release.md b/matrix-spreadsheet/release.md index 2b94873b2..4ccfeff21 100644 --- a/matrix-spreadsheet/release.md +++ b/matrix-spreadsheet/release.md @@ -4,7 +4,6 @@ **Refactoring, code quality, and usability improvements** ### Breaking Changes -- removed `'Sheet1'` default from `String sheet` overloads in `SpreadsheetImporter`; callers relying on the implicit default should use the new whole-sheet convenience imports or pass an explicit sheet name - `Sheet.name` field type tightened from `Object` to `String` - `Importer` interface: `startColumn`/`endColumn` parameters renamed to `startCol`/`endCol` for consistency diff --git a/matrix-spreadsheet/req/v2.4.0-roadmap.md b/matrix-spreadsheet/req/v2.4.0-roadmap.md index 33e362114..77149c12a 100644 --- a/matrix-spreadsheet/req/v2.4.0-roadmap.md +++ b/matrix-spreadsheet/req/v2.4.0-roadmap.md @@ -40,16 +40,16 @@ in the v2.3.1 code review. - **Consideration**: Modern Java practice would use Optional - **Note**: This is a breaking API change; defer to v3.0 if needed -### 2.3 [ ] Consider more specific exception types in FileUtil +### 2.3 [x] Consider more specific exception types in FileUtil - **File**: `src/main/groovy/se/alipsa/matrix/spreadsheet/FileUtil.groovy` - **Issue**: Throws generic `Exception` in several places -- **Note**: Low priority; current behavior works +- **Fix**: Replaced with `SpreadsheetImportException` (see Priority 4 below) --- ## Priority 3: Bug Fixes & Code Quality (from v2.3.1 review) -### 3.1 [ ] Remove dead code and wasted I/O in `FExcelExporter.exportExcel` +### 3.1 [x] Remove dead code and wasted I/O in `FExcelExporter.exportExcel` - **File**: `src/main/groovy/se/alipsa/matrix/spreadsheet/fastexcel/FExcelExporter.groovy` lines 83–97 - **Issue**: When the target file already exists, the method opens a `ReadableWorkbook` (allocating I/O resources), runs sheet-collision avoidance logic, and then unconditionally throws `IllegalArgumentException`. The collision-avoidance `if/while` block is completely dead — the throw always fires regardless. The workbook is opened and closed for nothing. - **Fix**: Remove the entire `try` block. Replace the `if (file.exists() && file.length() > 0)` branch with a direct throw: @@ -59,7 +59,7 @@ in the v2.3.1 code review. } ``` -### 3.2 [ ] Replace undeclared `commons-io` transitive dependency in `FOdsImporter` +### 3.2 [x] Replace undeclared `commons-io` transitive dependency in `FOdsImporter` - **File**: `src/main/groovy/se/alipsa/matrix/spreadsheet/fastods/FOdsImporter.groovy` line 5, 197 - **Issue**: `IOUtils.toByteArray(is)` is used via `org.apache.commons.io.IOUtils`, which is only available as a transitive dependency from `fast.excel.reader`. It is not declared in `build.gradle`. If the transitive chain changes this dependency will break silently. - **Fix**: Replace with Groovy's built-in `is.bytes` and remove the import: @@ -71,51 +71,51 @@ in the v2.3.1 code review. byte[] bytes = is.bytes ``` -### 3.3 [ ] Make `SpreadsheetImporter.validateNotNull` private +### 3.3 [x] Make `SpreadsheetImporter.validateNotNull` private - **File**: `src/main/groovy/se/alipsa/matrix/spreadsheet/SpreadsheetImporter.groovy` line 291 - **Issue**: `static void validateNotNull(Object paramVal, String paramName)` has default (public) Groovy visibility and is therefore part of the public API surface, which is unintentional. - **Fix**: Add `private` modifier. -### 3.4 [ ] Add `@CompileStatic` to `FExcelExporter` or document why it is absent +### 3.4 [x] Add `@CompileStatic` to `FExcelExporter` or document why it is absent - **File**: `src/main/groovy/se/alipsa/matrix/spreadsheet/fastexcel/FExcelExporter.groovy` lines 21–22 - **Issue**: `@CompileStatic` is commented out with only a brief note about a module access error on `GenericStyleSetter`. The entire exporter runs with dynamic dispatch as a result. - **Fix**: Investigate whether the access error is avoidable (e.g. `--add-opens` JVM arg, or avoiding the offending call). If unavoidable, expand the comment to explain the root cause and link to the upstream fastexcel issue so the limitation is tracked rather than forgotten. -### 3.5 [ ] Add class-level GroovyDoc to `SpreadsheetImporter` +### 3.5 [x] Add class-level GroovyDoc to `SpreadsheetImporter` - **File**: `src/main/groovy/se/alipsa/matrix/spreadsheet/SpreadsheetImporter.groovy` - **Issue**: `SpreadsheetImporter` is the primary import facade but has no class-level documentation. `SpreadsheetWriter` has thorough class doc including a usage example; `SpreadsheetImporter` should follow the same pattern. -### 3.6 [ ] Fix `@param` mismatches in `SpreadsheetImporter.importOds` / `importExcel` +### 3.6 [x] Fix `@param` mismatches in `SpreadsheetImporter.importOds` / `importExcel` - **File**: `src/main/groovy/se/alipsa/matrix/spreadsheet/SpreadsheetImporter.groovy` lines 41–123 - **Issue**: The `importOds(URL url, ...)` and `importExcel(URL url, ...)` overloads copy-paste the doc block from the file-path variants and say `@param file the ods or excel file to import`, but the parameter is `url`, not `file`. Several overloads also have no GroovyDoc at all. - **Fix**: Correct the `@param` tag to `@param url` with an appropriate description; add missing GroovyDoc to undocumented overloads. -### 3.7 [ ] Add GroovyDoc to `SpreadsheetWriter.writeSheets(Map params)` +### 3.7 [x] Add GroovyDoc to `SpreadsheetWriter.writeSheets(Map params)` - **File**: `src/main/groovy/se/alipsa/matrix/spreadsheet/SpreadsheetWriter.groovy` line 210 - **Issue**: The map-params overload is undocumented — which keys are valid, what types, which are required? - **Fix**: Add a `@param params` block listing `file` (File), `data` (List<Matrix>), and `sheetNames` (List<String>), matching the style used for the other overloads. -### 3.8 [ ] Standardise `startCol`/`endCol` parameter naming across `Importer` and `SpreadsheetImporter` +### 3.8 [x] Standardise `startCol`/`endCol` parameter naming across `Importer` and `SpreadsheetImporter` - **Files**: `src/main/groovy/se/alipsa/matrix/spreadsheet/Importer.groovy`, `SpreadsheetImporter.groovy` - **Issue**: Some overloads use `startColumn`/`endColumn` (e.g. `importSpreadsheet(String, int, int, int, String, String, boolean)`) while the majority use `startCol`/`endCol`. The inconsistency appears in both the interface and the facade. - **Fix**: Standardise on `startCol`/`endCol` throughout. -### 3.9 [ ] Fix misleading GroovyDoc on `FExcelExporter.exportExcel` overloads +### 3.9 [x] Fix misleading GroovyDoc on `FExcelExporter.exportExcel` overloads - **File**: `src/main/groovy/se/alipsa/matrix/spreadsheet/fastexcel/FExcelExporter.groovy` lines 29–78 - **Issue**: All four `exportExcel` overloads claim "if the excel file exists and is not empty, a new sheet will be added to the excel". Since v2.3.0 the method unconditionally throws `IllegalArgumentException` when the file already exists. The GroovyDoc is stale and misleading. - **Fix**: Update the GroovyDoc to state that existing files cause an `IllegalArgumentException` and direct users to `SpreadsheetWriter` or `FExcelAppender` for append/replace behavior. -### 3.10 [ ] Add `@CompileStatic` to `ValueExtractor` +### 3.10 [x] Add `@CompileStatic` to `ValueExtractor` - **File**: `src/main/groovy/se/alipsa/matrix/spreadsheet/ValueExtractor.groovy` - **Issue**: The abstract utility class lacks `@CompileStatic`, running with dynamic dispatch despite being pure type-conversion logic. This is inconsistent with the project's static-compilation preference. - **Fix**: Add `@CompileStatic` and resolve any typing issues that surface. -### 3.11 [ ] Tighten `Sheet.name` field type in `fastods` +### 3.11 [x] Tighten `Sheet.name` field type in `fastods` - **File**: `src/main/groovy/se/alipsa/matrix/spreadsheet/fastods/Sheet.groovy` - **Issue**: The `name` field and `getName()` return type are `Object`, but the class only ever stores `String` or `Integer` (via the typed `setName` overloads). Using `Object` hides the actual domain from callers and skips IDE assistance. - **Fix**: Store name as `String` internally ( callers can pass `Integer` and it is converted immediately in `setName(Number)` ), or split into `sheetName` / `sheetNumber` fields. -### 3.12 [ ] Improve error messages when a named sheet does not exist +### 3.12 [x] Improve error messages when a named sheet does not exist - **Files**: `src/main/groovy/se/alipsa/matrix/spreadsheet/fastexcel/FExcelImporter.groovy` lines 160, 172; `fastods/FOdsImporter.groovy` equivalent paths - **Issue**: `workbook.findSheet(sheetName).orElseThrow()` and `odsDataReader.readOds(..., sheetName, ...)` produce generic `NoSuchElementException: No value present` (or null-sheet errors) instead of naming the missing sheet. This compounds the `'Sheet1'` default problem (4.3) by giving users no clue which sheet name was sought. - **Fix**: Supply explicit error messages such as `"Sheet '${sheetName}' does not exist in the workbook"`. @@ -124,12 +124,12 @@ in the v2.3.1 code review. ## Priority 4: Usability Improvements -### 4.1 [ ] Add `File`-based overloads to `SpreadsheetImporter` +### 4.1 [x] Add `File`-based overloads to `SpreadsheetImporter` - **Files**: `src/main/groovy/se/alipsa/matrix/spreadsheet/SpreadsheetImporter.groovy`, `Importer.groovy` - **Issue**: `SpreadsheetWriter.write` accepts `File` as its primary target type, but every `SpreadsheetImporter.importSpreadsheet` overload takes `String` file paths. Users with a `File` object must call `.absolutePath` at every call site. - **Fix**: Add `File`-accepting overloads that delegate via `file.absolutePath`, matching the writer's API shape. -### 4.2 [ ] Add whole-sheet convenience import (auto-detect dimensions) +### 4.2 [x] Add whole-sheet convenience import (auto-detect dimensions) - **File**: `src/main/groovy/se/alipsa/matrix/spreadsheet/SpreadsheetImporter.groovy` - **Issue**: Every import method requires explicit `endRow` and `endCol`. The `SpreadsheetFormatProvider` already auto-detects these via `SpreadsheetReader.findLastRow`/`findLastCol`, but this is not exposed through `SpreadsheetImporter`. Importing a whole sheet currently requires 3–4 extra method calls (open reader, find last row, find last col, close, then import). - **Fix**: Add convenience overloads that use `SpreadsheetReader` internally to detect dimensions: @@ -139,12 +139,10 @@ in the v2.3.1 code review. static Matrix importSpreadsheet(String file, String sheetName, boolean firstRowAsColNames = true) ``` -### 4.3 [ ] Fix `'Sheet1'` hardcoded default in static overloads -- **File**: `src/main/groovy/se/alipsa/matrix/spreadsheet/SpreadsheetImporter.groovy` lines 161, 185 -- **Issue**: `importSpreadsheet(String, String sheet = 'Sheet1', ...)` defaults to the literal string `"Sheet1"`. The map-based overload (line 225) correctly defaults to sheet index `1`. Files without a sheet named "Sheet1" silently fail via the static overloads. The v1.2.0 release notes say "Default to 1st sheet instead of 'Sheet1'" but only the map API was updated. -- **Fix**: Change the string default to a numeric sheet-index default (using the auto-detect approach from 4.2), or at minimum document the limitation clearly in GroovyDoc. +### 4.3 [N/A] Fix `'Sheet1'` hardcoded default in static overloads +- **Decision**: `'Sheet1'` is the universal default sheet name in both Excel and Calc; kept as-is. -### 4.4 [ ] Refresh README dependency versions and remove stale Log4j claim +### 4.4 [x] Refresh README dependency versions and remove stale Log4j claim - **File**: `matrix-spreadsheet/README.md` - **Issue**: The README references outdated versions (`matrix-core:3.6.0`, `groovy:5.0.4`, `matrix-spreadsheet:2.3.0`). It also lists **Log4j** as a third-party library (line 248), but the module migrated to `matrix-core` Logger in v2.3.0 and has no direct Log4j dependency. - **Fix**: Update version references to current aligned versions and remove the Log4j section. @@ -153,5 +151,5 @@ in the v2.3.1 code review. ## Priority 5: Release Checklist -- [ ] Remove SNAPSHOT suffix from build.gradle version when cutting release -- [ ] Update `release.md` v2.4.0 notes with current test count (124 tests) and completed fixes +- [x] Remove SNAPSHOT suffix from build.gradle version when cutting release +- [x] Update `release.md` v2.4.0 notes with current test count (124 tests) and completed fixes diff --git a/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/FileUtil.groovy b/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/FileUtil.groovy index 16ff95a64..72ac7ae23 100644 --- a/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/FileUtil.groovy +++ b/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/FileUtil.groovy @@ -24,15 +24,15 @@ class FileUtil { File excelFile URL url = getResourceUrl(filePath) if (url == null) { - throw new SpreadsheetImportException(filePath + " does not exist") + throw new SpreadsheetImportException("${filePath} does not exist") } try { excelFile = Paths.get(url.toURI()).toFile() } catch (URISyntaxException | RuntimeException e) { - throw new SpreadsheetImportException(filePath + " does not exist", e) + throw new SpreadsheetImportException("${filePath} does not exist", e) } if (!excelFile.exists()) { - throw new SpreadsheetImportException(filePath + " does not exist") + throw new SpreadsheetImportException("${filePath} does not exist") } return excelFile } diff --git a/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/SpreadsheetImporter.groovy b/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/SpreadsheetImporter.groovy index 526ed3085..24ca72478 100644 --- a/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/SpreadsheetImporter.groovy +++ b/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/SpreadsheetImporter.groovy @@ -177,7 +177,7 @@ class SpreadsheetImporter { * Import a spreadsheet by file path and sheet name. * * @param file the ods or excel file to import - * @param sheet the sheet name + * @param sheet the sheet name, defaults to 'Sheet1' * @param startRow the first row to include (the first row is row 1) * @param endRow the last row to include * @param startCol the first column to include (index starting with 1) @@ -186,7 +186,7 @@ class SpreadsheetImporter { * defaults to true * @return A Matrix corresponding to the spreadsheet data. */ - static Matrix importSpreadsheet(String file, String sheet, + static Matrix importSpreadsheet(String file, String sheet = 'Sheet1', int startRow = 1, int endRow, int startCol = 1, int endCol, boolean firstRowAsColNames = true) { @@ -202,7 +202,7 @@ class SpreadsheetImporter { * Import a spreadsheet by file path and sheet name using column names. * * @param file the ods or excel file to import - * @param sheet the sheet name + * @param sheet the sheet name, defaults to 'Sheet1' * @param startRow the first row to include (the first row is row 1) * @param endRow the last row to include * @param startCol the first column name to include (A is the first column name etc.) @@ -211,7 +211,7 @@ class SpreadsheetImporter { * defaults to true * @return A Matrix corresponding to the spreadsheet data. */ - static Matrix importSpreadsheet(String file, String sheet, + static Matrix importSpreadsheet(String file, String sheet = 'Sheet1', int startRow = 1, int endRow, String startCol = 'A', String endCol, boolean firstRowAsColNames = true) { @@ -226,18 +226,6 @@ class SpreadsheetImporter { .importSpreadsheet(file, sheet, startRow, endRow, startCol, endCol, firstRowAsColNames) } - /** - * - * @param params the named parameters i.e. - * file (a File or a String path), - * sheet (the sheet tab to import, an integer of String), - * startRow (an Integer), - * endRow (and integer), - * startCol (an Integer or String), - * endCol (an Integer or String), - * firstRowAsColNames (a boolean) - * @return A Matrix corresponding to the spreadsheet data. - */ // --- File-based overloads --- /** @@ -336,8 +324,32 @@ class SpreadsheetImporter { } } + /** + * Import an entire spreadsheet sheet by name using auto-detected dimensions. + * + * @param file the spreadsheet file to import + * @param sheetName the sheet name + * @param firstRowAsColNames whether to treat the first row as column names + * @return A Matrix corresponding to the spreadsheet data. + */ + static Matrix importSpreadsheet(File file, String sheetName, boolean firstRowAsColNames = true) { + importSpreadsheet(file.absolutePath, sheetName, firstRowAsColNames) + } + // --- Map-based import --- + /** + * + * @param params the named parameters i.e. + * file (a File or a String path), + * sheet (the sheet tab to import, an integer of String), + * startRow (an Integer), + * endRow (and integer), + * startCol (an Integer or String), + * endCol (an Integer or String), + * firstRowAsColNames (a boolean) + * @return A Matrix corresponding to the spreadsheet data. + */ static Matrix importSpreadsheet(Map params) { def fp = params['file'] validateNotNull(fp, 'file') diff --git a/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/fastods/FOdsImporter.groovy b/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/fastods/FOdsImporter.groovy index 17150d4b1..913354f0d 100644 --- a/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/fastods/FOdsImporter.groovy +++ b/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/fastods/FOdsImporter.groovy @@ -73,7 +73,7 @@ class FOdsImporter implements Importer { sheetName, startRow, endRow, startCol, endCol) } if (sheet == null) { - throw new IllegalArgumentException("Sheet '${sheetName}' does not exist in the workbook") + throw new NoSuchElementException("Sheet '${sheetName}' does not exist in the workbook") } return buildMatrix(sheet, firstRowAsColNames) } @@ -113,7 +113,7 @@ class FOdsImporter implements Importer { boolean firstRowAsColNames = true) { def sheet = odsDataReader.readOds(is, sheetName, startRow, endRow, startCol, endCol) if (sheet == null) { - throw new IllegalArgumentException("Sheet '${sheetName}' does not exist in the workbook") + throw new NoSuchElementException("Sheet '${sheetName}' does not exist in the workbook") } return buildMatrix(sheet, firstRowAsColNames) } diff --git a/matrix-spreadsheet/src/test/groovy/spreadsheet/SpreadsheetImporterTest.groovy b/matrix-spreadsheet/src/test/groovy/spreadsheet/SpreadsheetImporterTest.groovy index f62536900..38bbab79e 100644 --- a/matrix-spreadsheet/src/test/groovy/spreadsheet/SpreadsheetImporterTest.groovy +++ b/matrix-spreadsheet/src/test/groovy/spreadsheet/SpreadsheetImporterTest.groovy @@ -16,8 +16,11 @@ import se.alipsa.matrix.spreadsheet.Importer import se.alipsa.matrix.spreadsheet.SpreadsheetImporter import se.alipsa.matrix.spreadsheet.fastexcel.FExcelImporter +import se.alipsa.matrix.spreadsheet.fastods.FastOdsException + import java.io.File import java.io.InputStream +import java.util.NoSuchElementException import java.math.RoundingMode import java.time.LocalDate import java.time.format.DateTimeFormatter @@ -213,4 +216,58 @@ class SpreadsheetImporterTest { Matrix m = importSpreadsheet("Book1.xlsx", "Sheet1", true) book1ImportAssertions(m) } + + @Test + void testAutoDetectOdsByFile() throws Exception { + File file = new File(SpreadsheetImporterTest.class.getResource("/Book1.ods").toURI()) + Matrix m = importSpreadsheet(file, 1, true) + book1ImportAssertions(m) + } + + @Test + void testAutoDetectOdsByName() { + Matrix m = importSpreadsheet("Book1.ods", "Sheet1", true) + book1ImportAssertions(m) + } + + @Test + void testAutoDetectXlsxByFileAndSheetName() throws Exception { + File file = new File(SpreadsheetImporterTest.class.getResource("/Book1.xlsx").toURI()) + Matrix m = importSpreadsheet(file, "Sheet1", true) + book1ImportAssertions(m) + } + + @Test + void testFileBasedDelegationWithExplicitRange() throws Exception { + File file = new File(SpreadsheetImporterTest.class.getResource("/Book1.xlsx").toURI()) + Matrix m = importSpreadsheet(file, 1, 1, 12, 1, 4, true) + book1ImportAssertions(m) + } + + @Test + void testMissingSheetThrowsWithSheetName() { + assertThrows(NoSuchElementException.class, { + importSpreadsheet("Book1.xlsx", "NonExistentSheet", 1, 12, 1, 4, true) + }) + } + + @Test + void testMissingOdsSheetThrowsWithSheetName() { + assertThrows(FastOdsException.class, { + importSpreadsheet("Book1.ods", "NonExistentSheet", 1, 12, 1, 4, true) + }) + } + + @Test + void testImportSpreadsheetsWithFile() throws Exception { + File file = new File(SpreadsheetImporterTest.class.getResource("/Book2.xlsx").toURI()) + Map sheets = SpreadsheetImporter.importSpreadsheets(file, + [ + [sheetName: 'Sheet1', startRow: 3, endRow: 11, startCol: 2, endCol: 6, firstRowAsColNames: true], + [sheetName: 'Sheet2', startRow: 1, endRow: 12, startCol: 'A', endCol: 'D', firstRowAsColNames: true] + ]) + assertEquals(2, sheets.size()) + assertNotNull(sheets.Sheet1) + assertNotNull(sheets.Sheet2) + } } From f0cc35a14da01a912baad8211f13608b92928a86 Mon Sep 17 00:00:00 2001 From: pernyf Date: Wed, 6 May 2026 09:55:32 +0200 Subject: [PATCH 4/4] Fix overload ambiguity, dead code, and error message quoting - Remove unreachable null checks in FOdsImporter (OdsStreamDataReader already throws FastOdsException before returning null) - Remove default from sheetNumber in auto-detect overloads to prevent ambiguous dispatch; add explicit no-arg convenience methods instead - Document differing exception types for missing sheets (XLSX vs ODS) - Quote file paths in FileUtil error messages for readability - Rename test to reflect actual exception type (FastOdsException) Co-Authored-By: Claude Opus 4.6 --- .../alipsa/matrix/spreadsheet/FileUtil.groovy | 6 ++-- .../spreadsheet/SpreadsheetImporter.groovy | 33 ++++++++++++++++--- .../spreadsheet/fastods/FOdsImporter.groovy | 6 ---- .../SpreadsheetImporterTest.groovy | 2 +- 4 files changed, 32 insertions(+), 15 deletions(-) diff --git a/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/FileUtil.groovy b/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/FileUtil.groovy index 72ac7ae23..df5682178 100644 --- a/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/FileUtil.groovy +++ b/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/FileUtil.groovy @@ -24,15 +24,15 @@ class FileUtil { File excelFile URL url = getResourceUrl(filePath) if (url == null) { - throw new SpreadsheetImportException("${filePath} does not exist") + throw new SpreadsheetImportException("'${filePath}' does not exist") } try { excelFile = Paths.get(url.toURI()).toFile() } catch (URISyntaxException | RuntimeException e) { - throw new SpreadsheetImportException("${filePath} does not exist", e) + throw new SpreadsheetImportException("'${filePath}' does not exist", e) } if (!excelFile.exists()) { - throw new SpreadsheetImportException("${filePath} does not exist") + throw new SpreadsheetImportException("'${filePath}' does not exist") } return excelFile } diff --git a/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/SpreadsheetImporter.groovy b/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/SpreadsheetImporter.groovy index 24ca72478..90c6f575f 100644 --- a/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/SpreadsheetImporter.groovy +++ b/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/SpreadsheetImporter.groovy @@ -274,15 +274,25 @@ class SpreadsheetImporter { // --- Whole-sheet convenience imports (auto-detect dimensions) --- + /** + * Import the first sheet of a spreadsheet file using auto-detected dimensions. + * + * @param file the spreadsheet file to import + * @return A Matrix corresponding to the spreadsheet data. + */ + static Matrix importSpreadsheet(File file) { + importSpreadsheet(file, 1, true) + } + /** * Import an entire spreadsheet sheet using auto-detected dimensions. * * @param file the spreadsheet file to import - * @param sheetNumber the sheet index (1-based), defaults to 1 + * @param sheetNumber the sheet index (1-based) * @param firstRowAsColNames whether to treat the first row as column names * @return A Matrix corresponding to the spreadsheet data. */ - static Matrix importSpreadsheet(File file, int sheetNumber = 1, boolean firstRowAsColNames = true) { + static Matrix importSpreadsheet(File file, int sheetNumber, boolean firstRowAsColNames = true) { SpreadsheetReader.Factory.create(file).withCloseable { SpreadsheetReader reader -> int endRow = reader.findLastRow(sheetNumber) int endCol = reader.findLastCol(sheetNumber) @@ -290,15 +300,25 @@ class SpreadsheetImporter { } } + /** + * Import the first sheet of a spreadsheet using auto-detected dimensions. + * + * @param file the spreadsheet file path to import + * @return A Matrix corresponding to the spreadsheet data. + */ + static Matrix importSpreadsheet(String file) { + importSpreadsheet(file, 1, true) + } + /** * Import an entire spreadsheet sheet using auto-detected dimensions. * * @param file the spreadsheet file path to import - * @param sheetNumber the sheet index (1-based), defaults to 1 + * @param sheetNumber the sheet index (1-based) * @param firstRowAsColNames whether to treat the first row as column names * @return A Matrix corresponding to the spreadsheet data. */ - static Matrix importSpreadsheet(String file, int sheetNumber = 1, boolean firstRowAsColNames = true) { + static Matrix importSpreadsheet(String file, int sheetNumber, boolean firstRowAsColNames) { File spreadsheetFile = FileUtil.checkFilePath(file) SpreadsheetReader.Factory.create(spreadsheetFile).withCloseable { SpreadsheetReader reader -> int endRow = reader.findLastRow(sheetNumber) @@ -310,12 +330,15 @@ class SpreadsheetImporter { /** * Import an entire spreadsheet sheet by name using auto-detected dimensions. * + *

For XLSX files, a missing sheet throws {@link NoSuchElementException}. + * For ODS files, a missing sheet throws {@link se.alipsa.matrix.spreadsheet.fastods.FastOdsException}.

+ * * @param file the spreadsheet file path to import * @param sheetName the sheet name * @param firstRowAsColNames whether to treat the first row as column names * @return A Matrix corresponding to the spreadsheet data. */ - static Matrix importSpreadsheet(String file, String sheetName, boolean firstRowAsColNames = true) { + static Matrix importSpreadsheet(String file, String sheetName, boolean firstRowAsColNames) { File spreadsheetFile = FileUtil.checkFilePath(file) SpreadsheetReader.Factory.create(spreadsheetFile).withCloseable { SpreadsheetReader reader -> int endRow = reader.findLastRow(sheetName) diff --git a/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/fastods/FOdsImporter.groovy b/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/fastods/FOdsImporter.groovy index 913354f0d..e0d67d8e9 100644 --- a/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/fastods/FOdsImporter.groovy +++ b/matrix-spreadsheet/src/main/groovy/se/alipsa/matrix/spreadsheet/fastods/FOdsImporter.groovy @@ -72,9 +72,6 @@ class FOdsImporter implements Importer { sheet = odsDataReader.readOds(fis, sheetName, startRow, endRow, startCol, endCol) } - if (sheet == null) { - throw new NoSuchElementException("Sheet '${sheetName}' does not exist in the workbook") - } return buildMatrix(sheet, firstRowAsColNames) } @@ -112,9 +109,6 @@ class FOdsImporter implements Importer { int startCol = 1, int endCol, boolean firstRowAsColNames = true) { def sheet = odsDataReader.readOds(is, sheetName, startRow, endRow, startCol, endCol) - if (sheet == null) { - throw new NoSuchElementException("Sheet '${sheetName}' does not exist in the workbook") - } return buildMatrix(sheet, firstRowAsColNames) } diff --git a/matrix-spreadsheet/src/test/groovy/spreadsheet/SpreadsheetImporterTest.groovy b/matrix-spreadsheet/src/test/groovy/spreadsheet/SpreadsheetImporterTest.groovy index 38bbab79e..4cc4ef663 100644 --- a/matrix-spreadsheet/src/test/groovy/spreadsheet/SpreadsheetImporterTest.groovy +++ b/matrix-spreadsheet/src/test/groovy/spreadsheet/SpreadsheetImporterTest.groovy @@ -252,7 +252,7 @@ class SpreadsheetImporterTest { } @Test - void testMissingOdsSheetThrowsWithSheetName() { + void testMissingOdsSheetThrowsFastOdsException() { assertThrows(FastOdsException.class, { importSpreadsheet("Book1.ods", "NonExistentSheet", 1, 12, 1, 4, true) })