Skip to content

feat: add Character and Year converters (#1017) - #1087

Open
BigDataDZ wants to merge 8 commits into
apache:mainfrom
BigDataDZ:feat/character-year-converters
Open

BigDataDZ wants to merge 8 commits into
apache:mainfrom
BigDataDZ:feat/character-year-converters

Conversation

@BigDataDZ

Copy link
Copy Markdown

Purpose of the pull request

Implements two commonly used JDK 8 compatible converters requested by the community task #1017:
java.lang.Character and java.time.Year. Proposal comments:
#1017 (comment) and the amendment below it
(UUID / YearMonth / Instant were proposed by other volunteers in parallel and are left to
them).

Related: #1017

What's changed?

  • CharacterStringConverter (org.apache.fesod.sheet.converters.charconverter): bidirectional
    Character <-> STRING. Reading an empty string yields null; strings longer than one character
    throw (wrapped into ExcelDataConvertException by the framework) instead of being silently
    truncated.
  • YearStringConverter (org.apache.fesod.sheet.converters.year): bidirectional Year <->
    STRING, default format uuuu, honors @DateTimeFormat custom patterns and
    GlobalConfiguration.locale.

Both follow the existing *StringConverter pattern (e.g. LocalTimeStringConverter) and are
registered in DefaultConverterLoader under putAllConverter, putWriteConverter and
putWriteStringConverter, 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

  • I have read the Contributor Guide.
  • I have written the necessary doc or comment.
  • I have added the necessary unit tests and all cases have passed.

Signed-off-by: BigDataDZ <76271875+BigDataDZ@users.noreply.github.com>
Signed-off-by: BigDataDZ <76271875+BigDataDZ@users.noreply.github.com>
Comment thread fesod-sheet/src/main/java/org/apache/fesod/sheet/util/DateUtils.java Outdated
Signed-off-by: BigDataDZ <76271875+BigDataDZ@users.noreply.github.com>
@BigDataDZ

Copy link
Copy Markdown
Author

Done in 254a11e — renamed to DateUtils.defaultYearFormat, following the sibling fields
defaultDateFormat / defaultLocalDateFormat, and parseYear / format(Year, ...) now fall
back to it when no explicit format is given, so it works as a global default just like the other
two. Tests re-run green (11/11 targeted, 927/927 full suite).

@bengbengbalabalabeng

Copy link
Copy Markdown
Contributor

It is recommended to add tests for the following scenarios to cover the complete behavior of YearStringConverter / DateUtils:

  • DateUtils#parseYear() / format(Year)
  • Locale's impact on formatting(format = "G y",Locale.CHINA -> "公元 2026",Locale.US -> "AD 2026")

Signed-off-by: BigDataDZ <76271875+BigDataDZ@users.noreply.github.com>
@BigDataDZ

Copy link
Copy Markdown
Author

Added in faf2207, covering both layers:

  1. DateUtilsTest — direct coverage for DateUtils#parseYear / #format(Year, ...):
    default yyyy parse and format (with null and explicit locales), plus a custom pattern
    (uu).
  2. Locale coverage — format(Year.of(2026), "G y", Locale.CHINA) → 公元 2026 and
    Locale.US → AD 2026, mirrored through the converter in
    YearStringConverterTest#convertToExcelDataRespectsLocaleForEraPatterns (a
    GlobalConfiguration with Locale.CHINA / Locale.US and a @DateTimeFormat pattern of
    G y).

Also adjusted the existing custom-format test: the previous MM/yyyy pattern is invalid for a
bare Year (no MonthOfYear field — UnsupportedTemporalTypeException), replaced with
'Y'yyyy → Y2026.

Targeted tests 79/79, full fesod-sheet suite 933/933 green.

@bengbengbalabalabeng bengbengbalabalabeng left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@nkuprins

Copy link
Copy Markdown
Contributor

Year is registered with putWriteStringConverter but not with putWriteConverter?

Signed-off-by: BigDataDZ <76271875+BigDataDZ@users.noreply.github.com>
@BigDataDZ

Copy link
Copy Markdown
Author

Good catch @nkuprins — that was an oversight, fixed in 7986ca1:
putWriteConverter(new YearStringConverter()) added alongside putWriteStringConverter.

Without it, a Year field failed on the xlsx write path with
ExcelWriteDataConvertException: Can not find 'Converter' support class Year. (CSV write and
reads were unaffected, which is why the converter-level unit tests did not catch it).

Added a full round-trip regression (yearFieldRoundTripsThroughFesodSheet — a Year field
written to xlsx and read back): red without the registration, green with it. Full fesod-sheet
suite green (934/934).

Signed-off-by: BigDataDZ <76271875+BigDataDZ@users.noreply.github.com>
Comment on lines +83 to +93
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();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same advice: YearStringConverterTest#yearFieldRoundTripsThroughFesodSheet()

Signed-off-by: BigDataDZ <76271875+BigDataDZ@users.noreply.github.com>
@BigDataDZ

Copy link
Copy Markdown
Author

Both suggestions addressed in 9683bf7:

  • The field round trips moved out of the unit test classes into a dedicated integration test,
    CharacterAndYearFieldRoundTripTest — extends AbstractExcelTest, parameterized with
    @ExcelFormatSource (XLSX / XLS / CSV) and built on RoundTripHelper#writeAndRead.
  • As a bonus, the parameterization now covers XLS and CSV too — legs the previous
    single-format tests did not exercise.

YearStringConverterTest / CharacterStringConverterTest remain as pure unit tests.
Full fesod-sheet suite green.

Comment on lines +40 to +41
* The xlsx/xls legs exercise the wildcard write key ((type, null)); the csv leg exercises the
* (type, STRING) key.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants