feat: add WriteSheetWorkbookWriteHandler to set the order of sheets #411
Conversation
...xcel-core/src/main/java/cn/idev/excel/write/handler/impl/WriteSheetWorkbookWriteHandler.java
Outdated
Show resolved
Hide resolved
wangmiscoding
left a comment
There was a problem hiding this comment.
Hello, I think the name "WriteSheetWorkbookWriteHandler" might need reconsideration. For example, what about "SortSheetHandler"?
This class will be used for 这个类可以用于后续对 |
OK,You make a good point |
# Conflicts: # fastexcel/src/main/java/cn/idev/excel/write/handler/impl/WriteSheetWorkbookWriteHandler.java
There was a problem hiding this comment.
Pull Request Overview
This pull request adds functionality to control the order of sheets in Excel workbooks by introducing a new WriteSheetWorkbookWriteHandler that automatically sorts sheets according to their sheetNo values.
Key changes:
- Adds a new handler that sorts sheets by their sheet number after workbook processing
- Integrates the handler into the default handler loader for both XLS and XLSX formats
- Includes comprehensive test coverage for various sheet ordering scenarios
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| WriteSheetWorkbookWriteHandler.java | New handler that implements sheet ordering logic using Workbook.setSheetOrder() |
| DefaultWriteHandlerLoader.java | Registers the new sheet ordering handler for XLS and XLSX formats |
| OrderConstant.java | Adds constant for sheet ordering execution priority |
| WriteSheetTest.java | Test suite covering normal, edge case, and negative number scenarios for sheet ordering |
| WriteSheetData.java | Simple test data model for sheet ordering tests |
Comments suppressed due to low confidence (2)
fastexcel/src/main/java/cn/idev/excel/write/handler/impl/WriteSheetWorkbookWriteHandler.java:35
- [nitpick] The variable name 'sheetNoSortList' is redundant since it contains 'sort' but the list is created by sorting. Consider renaming to 'sortedSheetNumbers' or 'orderedSheetKeys' for clarity.
List<Integer> sheetNoSortList =
fastexcel-test/src/test/java/cn/idev/excel/test/core/writesheet/WriteSheetTest.java:68
- The test only verifies data size but doesn't validate that sheets are actually in the correct order. Consider adding assertions to verify sheet names or positions match the expected sorted order.
Assertions.assertEquals(dataMap.get(sheetNoList.get(i)), sheetDataList.size());
| continue; | ||
| } | ||
| // set the order of sheet. | ||
| workbook.setSheetOrder(writeSheetHolder.getSheetName(), pos++); |
There was a problem hiding this comment.
The setSheetOrder method may fail if the sheet name doesn't exist in the workbook, but there's no error handling. Consider adding a try-catch block or validating sheet existence before calling this method.
psxjoy
left a comment
There was a problem hiding this comment.
LGTM
If no one has questions, this PR will merge in 24hours.
Purpose of the pull request
Close: #409
What's changed?
WriteSheetWorkbookWriteHandlerinDefaultWriteHandlerLoader#loadDefaultHandler()for xls and xlsx type.Workbook#setSheetOrder()method in theWriteSheetWorkbookWriteHandler#afterWorkbookDispose()method to sort according to the sheetNo value.Checklist