Copilot commented on code in PR #958: URL: https://github.com/apache/fesod/pull/958#discussion_r3609545422
########## .github/instructions/code-review.instructions.md: ########## @@ -0,0 +1,71 @@ +--- +applyTo: "**/*.java" +--- + +# Code Review Instructions for Apache Fesod + +## Priority Checks + +### 1. Java 8 Source Compatibility + +The project targets Java 8 (`source/target = 1.8`) but CI runs on JDK 8 through 25. Review for: + +- **No Java 9+ APIs**: Methods like `List.of()`, `Map.of()`, `String.strip()`, `InputStream.readAllBytes()` do not exist in Java 8. Use `Collections.singletonList()`, `new HashMap<>()`, `String.trim()`, or Apache Commons equivalents instead. +- **`java.sql.Date.toInstant()` / `java.sql.Time.toInstant()`**: These throw `UnsupportedOperationException` on Java 9+ because the types are date-only/time-only. When converting `java.util.Date` subclasses, always use `instanceof` checks and call `toLocalDate()` / `toLocalTime()` instead. This applies to both `WriteCellData` and `CsvCell` code paths — keep them consistent. +- **No `var` keyword**: Java 8 does not support `var`. Always declare explicit types. + +### 2. Immutability & Defensive Copies + +Builder methods that accept or return mutable collections must protect internal state: + +- **`head(Consumer)` pattern**: When `AbstractParameterBuilder.head(Consumer<HeadBuilder>)` stores the result of `DefaultHeadBuilder.define()`, it must wrap it with `toMutableListIfNecessary()` to create a defensive copy. Otherwise, `ExcelHeadProperty.initHeadRowNumber()` will mutate the builder's internal list during write, corrupting subsequent reuses. +- **General rule**: Any builder method that stores a collection from an external source must copy it. Any getter that returns an internal collection should document whether it's modifiable. + +### 3. Input Validation & Boundary Values + +- **Negative values**: Validation logic must handle all invalid values, not just sentinel defaults. For example, `@FreezePane(leftmostColumn = -1)` uses `-1` as "use default", but `leftmostColumn = -5` must also fall back to default — check `value == null || value < 0`, not just `value == -1`. +- **Null safety**: Public API entry points must validate null inputs and fail fast with a clear `IllegalArgumentException` rather than NPE deep in the call stack. + +### 4. Resource & Lifecycle Management + +- **ThreadLocal cleanup**: When setting thread-local state (e.g., `Biff8EncryptionKey.setCurrentUserPassword()`), do not clear it prematurely in a `finally` block if the state is needed by a later phase. The cleanup must happen only after all consumers have finished — typically in a dedicated `clearXxx()` method called at the end of the overall operation. +- **Try-with-resources**: Always use try-with-resources for `ExcelWriter`, `ExcelReader`, `Workbook`, `OutputStream`, `Writer`. Never leave workbook resources unclosed. +- **File handles**: After writing Excel files via POI, ensure the workbook is fully closed before the file handle is released. + +### 5. API Consistency Across Code Paths + +The same operation may be implemented in multiple places. When fixing a bug in one path, check all parallel paths: + +- `WriteCellData` vs `CsvCell`: Both handle date conversion. A fix in one must be mirrored in the other. +- `FesodSheet` vs `FastExcel` vs `EasyExcel`: These are aliases. A behavior change in the modern API must not break legacy aliases. +- `ExcelWriterBuilder` vs `ExcelReaderBuilder`: Symmetric builder patterns should stay symmetric. + +### 6. Test Quality + +- **Encoding consistency**: When tests write files, always specify the charset explicitly. Use `Files.newBufferedWriter(path, StandardCharsets.UTF_8)` instead of `new FileWriter(file)` (platform-default charset). The test must match the encoding configured in the production code (e.g., `CsvWorkbook` uses UTF-8). +- **Deprecated APIs in tests**: Avoid `new java.util.Date(year-1900, month, day)` — use `Calendar` or `java.time` types converted via `Date.from()`. Deprecated constructors generate warnings that pollute build output. +- **Tag selection**: Use the correct `@Tag`: + - `@Tag(Tags.UNIT)` for pure logic (no file I/O) + - `@Tag(Tags.ROUND_TRIP)` for write-then-read integration tests + - `@Tag(Tags.FORMAT)` for CSV/charset/date format tests + - `@Tag(Tags.WRITE)` for write-only tests +- **Real-file verification**: For bug fixes that affect file output, add at least one test that writes a real file and reads it back (via `ExcelAssertions` or POI `WorkbookFactory`). Code-level assertions alone may miss integration issues. +- **Regression evidence**: When fixing a bug, verify the test actually fails without the fix (revert source, run test, confirm failure, restore fix). + +### 7. ASF & Project Conventions + +- **License header**: Every new `.java` file must have the ASF Apache 2.0 header (see `tools/spotless/license-header.txt`). Spotless will fail CI if missing. Review Comment: The license-header enforcement is attributed to Spotless here, but Spotless is only configured for formatting/import order. License headers are enforced by the Hawkeye workflow (see `.github/workflows/license-check.yml`). ########## .github/copilot-instructions.md: ########## @@ -0,0 +1,84 @@ +# Apache Fesod (Incubating) — Copilot Repository Instructions + +## Project Overview + +Apache Fesod (Incubating) is a Java library for processing spreadsheets (XLS/XLSX/CSV) without OOM, derived from Alibaba EasyExcel. It uses Apache POI under the hood with streaming SAX-based reading. The project is an Apache incubator project and must follow ASF conventions. + +- **Language**: Java 8+ (source/target = 1.8; CI builds/tests on JDK 8–25) +- **Build**: Maven 3.6.3 via `./mvnw` wrapper (project docs require 3.9+, but wrapper currently pins 3.6.3; both work) +- **CI matrix**: JDK 8, 11, 17, 21, 25 + +## Build & Test Commands + +```bash +# Build (skips tests by default) +./mvnw clean install -DskipTests + +# Run tests (must explicitly enable) +./mvnw clean package -Dmaven.test.skip=false -pl fesod-common,fesod-shaded,fesod-sheet,fesod-examples/fesod-sheet-examples + +# Format check / auto-format +./mvnw spotless:check +./mvnw spotless:apply + +# Run tests by tag +./mvnw test -Dtest.groups=unit # fast unit tests only +./mvnw test -Dtest.excludedGroups=fuzz # exclude slow fuzz tests + +# Full install with javadoc (CI validation) +./mvnw install -B -V +./mvnw javadoc:javadoc +``` + +## Code Style & Conventions + +- **Formatter**: Palantir Java Format (enforced by Spotless, no checkstyle) +- **Imports**: Static imports first (`#|` pattern), then regular; no unused imports +- **Indent**: 4 spaces +- **License header**: ASF Apache 2.0 header required on all `.java`, `.xml`, `.yml`, `.toml` files (see `tools/spotless/license-header.txt`) Review Comment: This points to `tools/spotless/license-header.txt` and implies Spotless enforces license headers for multiple file types, but the repo's license checking is done by the Hawkeye workflow (`.github/workflows/license-check.yml`). This should reference the actual CI check (and treat the header file as a template if desired). ########## .github/copilot-instructions.md: ########## @@ -0,0 +1,84 @@ +# Apache Fesod (Incubating) — Copilot Repository Instructions + +## Project Overview + +Apache Fesod (Incubating) is a Java library for processing spreadsheets (XLS/XLSX/CSV) without OOM, derived from Alibaba EasyExcel. It uses Apache POI under the hood with streaming SAX-based reading. The project is an Apache incubator project and must follow ASF conventions. + +- **Language**: Java 8+ (source/target = 1.8; CI builds/tests on JDK 8–25) +- **Build**: Maven 3.6.3 via `./mvnw` wrapper (project docs require 3.9+, but wrapper currently pins 3.6.3; both work) +- **CI matrix**: JDK 8, 11, 17, 21, 25 + +## Build & Test Commands + +```bash +# Build (skips tests by default) +./mvnw clean install -DskipTests + +# Run tests (must explicitly enable) +./mvnw clean package -Dmaven.test.skip=false -pl fesod-common,fesod-shaded,fesod-sheet,fesod-examples/fesod-sheet-examples + +# Format check / auto-format +./mvnw spotless:check +./mvnw spotless:apply + +# Run tests by tag +./mvnw test -Dtest.groups=unit # fast unit tests only +./mvnw test -Dtest.excludedGroups=fuzz # exclude slow fuzz tests + +# Full install with javadoc (CI validation) +./mvnw install -B -V +./mvnw javadoc:javadoc +``` + +## Code Style & Conventions + +- **Formatter**: Palantir Java Format (enforced by Spotless, no checkstyle) +- **Imports**: Static imports first (`#|` pattern), then regular; no unused imports +- **Indent**: 4 spaces +- **License header**: ASF Apache 2.0 header required on all `.java`, `.xml`, `.yml`, `.toml` files (see `tools/spotless/license-header.txt`) +- **Lombok**: Allowed; config in `lombok.config` (`toString.callSuper = CALL`, `equalsAndHashCode.callSuper = CALL`) +- **Commits**: English, format `type: description` (types: `fix`, `feat`, `refactor`, `test`, `docs`, `chore`, `style`, `dependency`) + +## Module Structure + +``` +fesod-parent/ # Root POM, dependency & plugin management +├── fesod-common/ # Zero-dependency utilities (org.apache.fesod.common.util) +├── fesod-shaded/ # Relocated Spring ASM/cglib (org.apache.fesod.shaded) +├── fesod-bom/ # BOM for downstream consumers +├── fesod-sheet/ # Core library: read/write XLS/XLSX/CSV via POI +│ ├── src/main/java/org/apache/fesod/sheet/ +│ │ ├── FesodSheet.java # Main entry: FesodSheet.read() / FesodSheet.write() +│ │ ├── analysis/ # Read pipeline (v03=XLS BIFF, v07=XLSX SAX, csv) +│ │ ├── write/ # Write pipeline (builder, executor, handler chains) +│ │ ├── metadata/ # Data models, builders, csv/ property/ +│ │ ├── converters/ # Type conversion framework (by Java type) +│ │ └── util/ # DateUtils, NumberUtils, WorkBookUtil, etc. +│ └── src/test/java/org/apache/fesod/sheet/ +│ └── testkit/ # Test infrastructure (NOT a separate module) +│ ├── Tags.java # @Tag constants: unit, round-trip, read, write, format, fuzz +│ ├── base/ # AbstractExcelTest (round-trip base) +│ ├── assertions/ # ExcelAssertions fluent API +│ └── builders/ # TestDataBuilder +└── fesod-examples/ # Usage examples +``` + +## Testing Conventions + +- **JUnit 5** with `@TestInstance(PER_CLASS)`, `@DisplayName(ReplaceUnderscores)` Review Comment: `@DisplayName(ReplaceUnderscores)` isn’t a JUnit 5 annotation. In this repo, PER_CLASS lifecycle and the ReplaceUnderscores display-name generator are configured globally via `junit-platform.properties`, so the instructions should reference the configuration rather than a non-existent annotation. ########## .github/instructions/code-review.instructions.md: ########## @@ -0,0 +1,71 @@ +--- +applyTo: "**/*.java" +--- + +# Code Review Instructions for Apache Fesod + +## Priority Checks + +### 1. Java 8 Source Compatibility + +The project targets Java 8 (`source/target = 1.8`) but CI runs on JDK 8 through 25. Review for: + +- **No Java 9+ APIs**: Methods like `List.of()`, `Map.of()`, `String.strip()`, `InputStream.readAllBytes()` do not exist in Java 8. Use `Collections.singletonList()`, `new HashMap<>()`, `String.trim()`, or Apache Commons equivalents instead. +- **`java.sql.Date.toInstant()` / `java.sql.Time.toInstant()`**: These throw `UnsupportedOperationException` on Java 9+ because the types are date-only/time-only. When converting `java.util.Date` subclasses, always use `instanceof` checks and call `toLocalDate()` / `toLocalTime()` instead. This applies to both `WriteCellData` and `CsvCell` code paths — keep them consistent. +- **No `var` keyword**: Java 8 does not support `var`. Always declare explicit types. + +### 2. Immutability & Defensive Copies + +Builder methods that accept or return mutable collections must protect internal state: + +- **`head(Consumer)` pattern**: When `AbstractParameterBuilder.head(Consumer<HeadBuilder>)` stores the result of `DefaultHeadBuilder.define()`, it must wrap it with `toMutableListIfNecessary()` to create a defensive copy. Otherwise, `ExcelHeadProperty.initHeadRowNumber()` will mutate the builder's internal list during write, corrupting subsequent reuses. +- **General rule**: Any builder method that stores a collection from an external source must copy it. Any getter that returns an internal collection should document whether it's modifiable. + +### 3. Input Validation & Boundary Values + +- **Negative values**: Validation logic must handle all invalid values, not just sentinel defaults. For example, `@FreezePane(leftmostColumn = -1)` uses `-1` as "use default", but `leftmostColumn = -5` must also fall back to default — check `value == null || value < 0`, not just `value == -1`. +- **Null safety**: Public API entry points must validate null inputs and fail fast with a clear `IllegalArgumentException` rather than NPE deep in the call stack. + +### 4. Resource & Lifecycle Management + +- **ThreadLocal cleanup**: When setting thread-local state (e.g., `Biff8EncryptionKey.setCurrentUserPassword()`), do not clear it prematurely in a `finally` block if the state is needed by a later phase. The cleanup must happen only after all consumers have finished — typically in a dedicated `clearXxx()` method called at the end of the overall operation. +- **Try-with-resources**: Always use try-with-resources for `ExcelWriter`, `ExcelReader`, `Workbook`, `OutputStream`, `Writer`. Never leave workbook resources unclosed. +- **File handles**: After writing Excel files via POI, ensure the workbook is fully closed before the file handle is released. + +### 5. API Consistency Across Code Paths + +The same operation may be implemented in multiple places. When fixing a bug in one path, check all parallel paths: + +- `WriteCellData` vs `CsvCell`: Both handle date conversion. A fix in one must be mirrored in the other. +- `FesodSheet` vs `FastExcel` vs `EasyExcel`: These are aliases. A behavior change in the modern API must not break legacy aliases. +- `ExcelWriterBuilder` vs `ExcelReaderBuilder`: Symmetric builder patterns should stay symmetric. + +### 6. Test Quality + +- **Encoding consistency**: When tests write files, always specify the charset explicitly. Use `Files.newBufferedWriter(path, StandardCharsets.UTF_8)` instead of `new FileWriter(file)` (platform-default charset). The test must match the encoding configured in the production code (e.g., `CsvWorkbook` uses UTF-8). +- **Deprecated APIs in tests**: Avoid `new java.util.Date(year-1900, month, day)` — use `Calendar` or `java.time` types converted via `Date.from()`. Deprecated constructors generate warnings that pollute build output. +- **Tag selection**: Use the correct `@Tag`: + - `@Tag(Tags.UNIT)` for pure logic (no file I/O) + - `@Tag(Tags.ROUND_TRIP)` for write-then-read integration tests + - `@Tag(Tags.FORMAT)` for CSV/charset/date format tests + - `@Tag(Tags.WRITE)` for write-only tests +- **Real-file verification**: For bug fixes that affect file output, add at least one test that writes a real file and reads it back (via `ExcelAssertions` or POI `WorkbookFactory`). Code-level assertions alone may miss integration issues. +- **Regression evidence**: When fixing a bug, verify the test actually fails without the fix (revert source, run test, confirm failure, restore fix). + +### 7. ASF & Project Conventions + +- **License header**: Every new `.java` file must have the ASF Apache 2.0 header (see `tools/spotless/license-header.txt`). Spotless will fail CI if missing. +- **Package naming**: All code must be under `org.apache.fesod.*`. Never use `com.alibaba.*` or `cn.idev.*` (legacy packages from EasyExcel era). +- **Lombok**: `toString.callSuper = CALL` and `equalsAndHashCode.callSuper = CALL` are enforced via `lombok.config`. Subclasses must call super. +- **No checked exceptions in public API**: Wrap checked exceptions in `ExcelGenerateException` or `ExcelAnalysisException` rather than declaring `throws` on builder methods. + +### 8. Common Pitfalls (from real bug fixes) + +| Pattern | Problem | Fix | +|---------|---------|-----| +| `value.toInstant()` on `java.sql.Date`/`Time` | `UnsupportedOperationException` on Java 9+ | Use `instanceof` + `toLocalDate()`/`toLocalTime()` | +| `value == -1` validation | Misses other negative values | Use `value == null \|\| value < 0` | Review Comment: In the table, the fix example uses a backslash-escaped `\|\|` inside a code span. In GitHub-flavored Markdown, backslashes are rendered literally inside backticks, so readers will see `\|\|` instead of `||`. ########## .github/copilot-instructions.md: ########## @@ -0,0 +1,84 @@ +# Apache Fesod (Incubating) — Copilot Repository Instructions + +## Project Overview + +Apache Fesod (Incubating) is a Java library for processing spreadsheets (XLS/XLSX/CSV) without OOM, derived from Alibaba EasyExcel. It uses Apache POI under the hood with streaming SAX-based reading. The project is an Apache incubator project and must follow ASF conventions. + +- **Language**: Java 8+ (source/target = 1.8; CI builds/tests on JDK 8–25) +- **Build**: Maven 3.6.3 via `./mvnw` wrapper (project docs require 3.9+, but wrapper currently pins 3.6.3; both work) +- **CI matrix**: JDK 8, 11, 17, 21, 25 + +## Build & Test Commands + +```bash +# Build (skips tests by default) +./mvnw clean install -DskipTests + +# Run tests (must explicitly enable) +./mvnw clean package -Dmaven.test.skip=false -pl fesod-common,fesod-shaded,fesod-sheet,fesod-examples/fesod-sheet-examples + +# Format check / auto-format +./mvnw spotless:check +./mvnw spotless:apply + +# Run tests by tag +./mvnw test -Dtest.groups=unit # fast unit tests only +./mvnw test -Dtest.excludedGroups=fuzz # exclude slow fuzz tests + +# Full install with javadoc (CI validation) +./mvnw install -B -V +./mvnw javadoc:javadoc +``` + +## Code Style & Conventions + +- **Formatter**: Palantir Java Format (enforced by Spotless, no checkstyle) +- **Imports**: Static imports first (`#|` pattern), then regular; no unused imports +- **Indent**: 4 spaces +- **License header**: ASF Apache 2.0 header required on all `.java`, `.xml`, `.yml`, `.toml` files (see `tools/spotless/license-header.txt`) +- **Lombok**: Allowed; config in `lombok.config` (`toString.callSuper = CALL`, `equalsAndHashCode.callSuper = CALL`) +- **Commits**: English, format `type: description` (types: `fix`, `feat`, `refactor`, `test`, `docs`, `chore`, `style`, `dependency`) + +## Module Structure + +``` +fesod-parent/ # Root POM, dependency & plugin management Review Comment: The module tree shows a `fesod-parent/` directory, but there is no such folder in the repository; `fesod-parent` is the root POM artifact at repo root. This could confuse contributors navigating the tree. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
