Repository navigation
fix: honor sheet-level autoTrim/autoStrip for CSV read and write - #1191
LuciferYang wants to merge 3 commits into
Conversation
BigDataDZ
left a comment
There was a problem hiding this comment.
LGTM - the sheet-over-workbook precedence matches the holder chain convention (sheet parameters override workbook ones), and moving dealRecord to currentReadHolder() finally aligns CSV with what the xls and xlsx readers already do. The trim expression semantics are preserved exactly (autoStrip still forces trimming).
One small test suggestion: the suite now covers sheet-level overrides (testSheetAutoTrim / testSheetAutoStrip / testWriteSheetAutoTrim) and the default, but not the explicit fallback branch - a case where autoTrim(false) is set only on the workbook parameter and the sheet flag stays null, asserting trimming is actually off in the output. That is the readSheet.getAutoTrim() != null ? ... : readWorkbook.getAutoTrim() fallback path, and locking it protects against a future "just read the sheet flag" simplification. One parameterized case each for read and write would do.
|
@BigDataDZ The workbook-only fallback is already tested on both sides. |
|
You are right - |
Purpose of the pull request
Closed: #1190
csv()returns a builder whose parameter object is theReadSheet/WriteSheet, soautoTrim()andautoStrip()called aftercsv()are stored on the sheet. The CSV builders only read the workbook flags when they configureCSVFormattrimming, and the CSV read executor takes the flags from the workbook holder. So.csv().autoTrim(false)and.sheet().autoTrim(false)on a CSV file are ignored, on both read and write.What's changed?
CsvReaderBuilder.buildExcelReader()andCsvWriterBuilder.buildExcelWriter()use the sheet flags when they are set and fall back to the workbook flags otherwise.CsvExcelReadExecutor.dealRecord()readsautoTrim/autoStripfromcurrentReadHolder(), the sheet holder, as the xls and xlsx readers already do. This also makes.sheet().autoTrim(false)work for CSV, which never goes throughCsvReaderBuilder.Nothing changes for code that only sets the flags at workbook level, because the sheet flags default to null and inherit from the workbook. Code that already calls
.csv().autoTrim(false)or.sheet().autoTrim(false)on CSV will now get untrimmed values, header cells included, which matches xlsx.On the write side
autoStrip(true)still maps toCSVFormattrim, so it removes ASCII whitespace only. That was already the case for the workbook-level flag.Tests in
CsvFormatTest:testSheetAutoTrim:.csv().autoTrim(false)and.sheet().autoTrim(false)keep" a "; a sheet flag overrides the opposite workbook flag for bothautoTrimandautoStrip.testSheetAutoStrip: withautoTrim(false),autoStrip(true)set aftercsv()removes U+3000 andautoStrip(false)keeps it.testWriteSheetAutoTrim: checks the exact written record for.csv().autoTrim(false)and for a sheet flag overriding the workbook flag.testAutoTrimnow also checks the written file and the value read back for the existing workbook-level case.Each of the three source changes, reverted on its own, makes at least one of these fail.
Checklist