Conversation
Signed-off-by: BigDataDZ <76271875+BigDataDZ@users.noreply.github.com>
Signed-off-by: BigDataDZ <76271875+BigDataDZ@users.noreply.github.com>
Signed-off-by: BigDataDZ <76271875+BigDataDZ@users.noreply.github.com>
|
Done in 254a11e — renamed to |
|
It is recommended to add tests for the following scenarios to cover the complete behavior of
|
Signed-off-by: BigDataDZ <76271875+BigDataDZ@users.noreply.github.com>
|
Added in faf2207, covering both layers:
Also adjusted the existing custom-format test: the previous Targeted tests 79/79, full fesod-sheet suite 933/933 green. |
|
|
Signed-off-by: BigDataDZ <76271875+BigDataDZ@users.noreply.github.com>
|
Good catch @nkuprins — that was an oversight, fixed in 7986ca1: Without it, a Added a full round-trip regression ( |
Signed-off-by: BigDataDZ <76271875+BigDataDZ@users.noreply.github.com>
| void characterFieldRoundTripsThroughFesodSheet() throws Exception { | ||
| File file = File.createTempFile("fesod-character", ".xlsx"); | ||
| file.deleteOnExit(); | ||
| CharacterWriteData row = new CharacterWriteData(); | ||
| row.setFlag('A'); | ||
|
|
||
| FesodSheet.write(file, CharacterWriteData.class).sheet().doWrite(Collections.singletonList(row)); | ||
|
|
||
| List<CharacterReadData> rows = FesodSheet.read(file, CharacterReadData.class, new CharacterReadListener()) | ||
| .sheet() | ||
| .doReadSync(); |
There was a problem hiding this comment.
Suggest extending AbstractExcelTest and using its createTempFile(...) helper to cover multiple formats (e.g., XLSX, XLS, CSV). And using RoundTripHelper#writeAndRead(...) to simplify the boilerplate read/write code.
Additionally, it would be better to move this test into a dedicated integration test class rather than mixing it with pure unit tests.
There was a problem hiding this comment.
Same advice: YearStringConverterTest#yearFieldRoundTripsThroughFesodSheet()
Signed-off-by: BigDataDZ <76271875+BigDataDZ@users.noreply.github.com>
|
Both suggestions addressed in 9683bf7:
|
| * The xlsx/xls legs exercise the wildcard write key ((type, null)); the csv leg exercises the | ||
| * (type, STRING) key. |
There was a problem hiding this comment.
In read scenarios, xlsx/xls also override matching logic of the kind (JavaType, CellDataType), not just (JavaType, null). Therefore, this comment only describes the override situation on the write path and does not cover the behavior of the read path, which can easily lead to misunderstanding.
My personal suggestion is to remove this description entirely (or revise it into a more comprehensive explanation).
Purpose of the pull request
Implements two commonly used JDK 8 compatible converters requested by the community task #1017:
java.lang.Characterandjava.time.Year. Proposal comments:#1017 (comment) and the amendment below it
(
UUID/YearMonth/Instantwere proposed by other volunteers in parallel and are left tothem).
Related: #1017
What's changed?
CharacterStringConverter(org.apache.fesod.sheet.converters.charconverter): bidirectionalCharacter<-> STRING. Reading an empty string yieldsnull; strings longer than one characterthrow (wrapped into
ExcelDataConvertExceptionby the framework) instead of being silentlytruncated.
YearStringConverter(org.apache.fesod.sheet.converters.year): bidirectionalYear<->STRING, default format
uuuu, honors@DateTimeFormatcustom patterns andGlobalConfiguration.locale.Both follow the existing
*StringConverterpattern (e.g.LocalTimeStringConverter) and areregistered in
DefaultConverterLoaderunderputAllConverter,putWriteConverterandputWriteStringConverter, so they work on read, xlsx write and CSV write alike.Each converter ships with JUnit 5 unit tests (11 tests total: type keys, round-trips, default and
custom formats, and error cases).
Checklist