Copilot commented on code in PR #958:
URL: https://github.com/apache/fesod/pull/958#discussion_r3609535203
##########
.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, but builds on JDK 17+)
Review Comment:
The language bullet says the project "builds on JDK 17+", but the CI
workflow runs tests on JDK 8 as well (and the project targets Java 8). This is
misleading for contributors and for Copilot guidance.
##########
.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, but builds on JDK 17+)
+- **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
Review Comment:
The imports bullet includes "(`#|` pattern)", which doesn't correspond to a
recognizable project convention and makes the instruction unclear. The ordering
itself is fine, but the parenthetical should be removed or replaced with a
clear description.
##########
.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` |
+| `try { setThreadLocal(x); } finally { setThreadLocal(null); }` | Clears
state needed by later phase | Remove premature cleanup; clean up at end of full
operation |
+| `parameter().setHead(DefaultHeadBuilder.define(c))` | Stores internal list
reference | Wrap with `toMutableListIfNecessary()` |
+| `new FileWriter(file)` in tests | Platform-default charset | Use
`Files.newBufferedWriter(path, UTF_8)` |
Review Comment:
In the pitfalls table, the suggested fix uses `UTF_8` but the recommended
API earlier uses `StandardCharsets.UTF_8`. Using `StandardCharsets.UTF_8` here
is clearer and copy/paste-safe.
--
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]
