fix: apply excel format for blank cells #160
Conversation
WalkthroughThe changes made in the pull request focus on enhancing the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/main/java/com/flowingcode/vaadin/addons/gridexporter/ExcelStreamResourceWriter.java (1)
344-346: LGTM: Null cell formatting implementationThe implementation correctly addresses the issue by applying Excel formats to null cells, maintaining consistent styling across the exported file.
Consider breaking down the buildCell method
The method is handling multiple responsibilities (value extraction, type checking, formatting). Consider splitting it into smaller, focused methods for better maintainability.
Example refactor:
- private void buildCell(Object value, Cell cell, Column<T> column, T item) { + private void buildCell(Object value, Cell cell, Column<T> column, T item) { + CellFormatting formatting = prepareCellFormatting(column, item); + applyCellValue(value, cell, formatting); + } + + private CellFormatting prepareCellFormatting(Column<T> column, T item) { + ValueProvider<T,String> provider = (ValueProvider<T, String>) ComponentUtil.getData(column, GridExporter.COLUMN_EXCEL_FORMAT_DATA_PROVIDER); + Map<String, CellStyle> cellStyles = getCellStyles(column); + String excelFormat = getExcelFormat(column, item, provider); + return new CellFormatting(provider, cellStyles, excelFormat); + } + + private void applyCellValue(Object value, Cell cell, CellFormatting formatting) { + if (value == null) { + applyNullCellFormatting(cell, formatting); + } else if (value instanceof Number) { + applyNumberFormatting(cell, (Number)value, formatting); + } // ... etc + }Also applies to: 349-351
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/main/java/com/flowingcode/vaadin/addons/gridexporter/ExcelStreamResourceWriter.java(2 hunks)
🔇 Additional comments (3)
src/main/java/com/flowingcode/vaadin/addons/gridexporter/ExcelStreamResourceWriter.java (3)
370-379: LGTM: Clean format resolution implementation
The method cleanly handles format resolution with clear precedence rules and good separation of concerns.
Line range hint 344-389: Verify test coverage for null cell formatting
The changes look good, but ensure there are unit tests covering:
- Null cells with different format providers
- Null cells with static formats
- Null cells without any format
Let's check the test coverage:
✅ Verification successful
Let me search for test files specifically related to Excel export and null values:
Let me check the content of some demo files that might contain test scenarios:
Let me check for integration or unit tests specifically:
Let me check the demo files for test data setup that might include null values:
Based on the review of the code and test files, I can now generate the final response:
Null cell formatting is adequately tested through demo scenarios
The codebase includes comprehensive test coverage for null cell formatting through demo scenarios:
GridExporterCustomTemplateDemoandGridExporterMultipleHeaderRowsDemoexplicitly test null values by randomly setting lastName to null:(Math.random() > 0.3 ? faker.name().lastName() : null)Personclass constructor supports null dates:favDateandworstDatecan be null- The implementation correctly handles various null scenarios with appropriate formatting:
- Blank cells for null values
- Default formats for non-null values
- Custom formats through format providers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for relevant test files
fd -e java -e groovy test
# Search for null value test cases
rg -A 5 "null.*cell|cell.*null"
Length of output: 9803
Script:
#!/bin/bash
# Search for test files containing Excel or export related tests
rg -l "Excel|export" src/test
# Search for test cases specifically handling null values in Excel export
rg -B 5 -A 5 "null.*value|value.*null" src/test
Length of output: 1906
Script:
#!/bin/bash
# Check demo files for test scenarios involving null values
cat src/test/java/com/flowingcode/vaadin/addons/gridexporter/GridExporterDemo.java
cat src/test/java/com/flowingcode/vaadin/addons/gridexporter/GridExporterCustomColumnsDemo.java
# Look for test data setup that might include null values
rg -A 10 "createPerson|setupGrid" src/test/java/com/flowingcode/vaadin/addons/gridexporter/
Length of output: 8725
Script:
#!/bin/bash
# Check integration tests
cat src/test/java/com/flowingcode/vaadin/addons/gridexporter/it/ViewIT.java
# Check concurrent export tests which might include null value scenarios
cat src/test/java/com/flowingcode/vaadin/addons/gridexporter/test/ConcurrentExportTests.java
# Look for test setup methods in all test files
rg -B 2 -A 10 "@Test|@Before" src/test/java/com/flowingcode/vaadin/addons/gridexporter/test/
Length of output: 31700
Script:
#!/bin/bash
# Look for test data setup in demo files
cat src/test/java/com/flowingcode/vaadin/addons/gridexporter/Person.java
# Search for grid setup and data population in demo files
rg -A 10 "grid.setItems|new Person" src/test/java/com/flowingcode/vaadin/addons/gridexporter/
Length of output: 22804
381-389: Consider thread safety implications
The shared cellStyles map is modified without synchronization. While the Grid component typically operates in a single-threaded context, it's worth documenting this assumption or adding synchronization if concurrent access is possible.
Let's check for potential concurrent access patterns:
Fixes #159
Summary by CodeRabbit