Visitar URL original
feat: add MonthDay converter family with round-trip tests by 213f08 · Pull Request #1325 · apache/fesod · GitHub
Skip to content

feat: add MonthDay converter family with round-trip tests - #1325

Open
213f08 wants to merge 1 commit into
apache:mainfrom
213f08:feat-monthday-converter
Open

213f08 wants to merge 1 commit into
apache:mainfrom
213f08:feat-monthday-converter

Conversation

@213f08

@213f08 213f08 commented Nov 10, 2026 •

Copy link
Copy Markdown

Purpose of the pull request

Related: #1017

Implements the java.time.MonthDay converter proposal from #1017, following the existing LocalDate / LocalTime converter family pattern. Rebased onto current main.

What's changed?

  • New org.apache.fesod.sheet.converters.monthday.MonthDayStringConverter:
    • Read: ISO-8601 --MM-dd via DateUtils.parseMonthDay, with a lenient MM-dd fallback
    • Write: ISO string via DateUtils.format(MonthDay, ...)
    • Honors @DateTimeFormat(...) when a pattern is configured; an empty or null pattern falls back to ISO. The configured GlobalConfiguration locale is passed to DateUtils, which reuses the locale-keyed DateTimeFormatter cache (matching LocalDateStringConverter / OffsetDateTimeStringConverter)
  • New DateUtils.parseMonthDay / DateUtils.format(MonthDay, ...) helpers, mirroring the existing parseLocalTime / format(LocalTime, ...) pattern
  • Registered in DefaultConverterLoader (all-converter, default-write and default-write-string maps), mirroring InstantStringConverter / YearStringConverter
  • Docs: MonthDay row added to the built-in converters table (en + zh-CN)
  • Tests (17 unit + 15 round-trip):
    • MonthDayStringConverterTest: ISO and lenient MM-dd parsing; rejection of invalid calendar dates (--02-30, month 13, full dates); leap day --02-29; ISO write; configured-pattern read/write, rejection of non-matching text, and locale-aware month names (Locale.US vs Locale.FRENCH); blank/null pattern falling back to ISO; boundary values round-trip (--01-01, --02-29, --12-31); converter registration for read and write
    • MonthDayConverterDataTest: write-then-read equality across xlsx / xls / csv for the default ISO format, a @DateTimeFormat("MM/dd") model, a nullable MonthDay column, multiple boundary values, and coexistence with a LocalDate field in the same row

This change was prepared with AI assistance and verified locally: full module suite fesod-common 60 + fesod-sheet 1849 tests, 0 failures; spotless:check clean.

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.

@Mikkey-f Mikkey-f 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.

Thanks @213f08 — the implementation matches the #1017 proposal (String-only, ISO --MM-dd with an MM-dd fallback, @DateTimeFormat support) and the registration mirrors YearStringConverter (all-converter, default-write by type, default-write by type + STRING).

Verification (worktree at ff051ba2, JDK 17, Windows)

Green:

  • targeted: MonthDayStringConverterTest 16/16 + MonthDayConverterDataTest 15/15 = 31
  • full: fesod-common 60 + fesod-sheet 1,820 = 1,880 tests, 0 failures
  • spotless:check ok

Red:

  • reverting only the DefaultConverterLoader registration (keeping the converter class) makes the registration test NPE and 12 of the 15 round-trip cases fail with Can not find 'Converter' support class MonthDay; restoring turns them green.

Non-blocking

  1. The configured locale is ignored for @DateTimeFormat patterns. The sibling string converters pass globalConfiguration.getLocale() down to DateUtils (YearStringConverter, LocalDateStringConverter, LocalTimeStringConverter), while DateTimeFormatter.ofPattern(format) here always uses the JVM default locale. Probe (not committed): on a zh_CN JVM, with GlobalConfiguration.locale = Locale.US and @DateTimeFormat("MMMM d"), both writes produced the JVM-default Chinese month name and reading March 15 threw DateTimeParseException; reading 三月 15 succeeded only because the default locale happened to match. Numeric patterns such as MM/dd are unaffected. To align, either add DateUtils.parseMonthDay / format(MonthDay, ...) helpers (they also reuse the locale-keyed formatter cache) or pass the locale when building the formatter, plus one locale round-trip test.
  2. Docs: after rebasing onto current main, please add a MonthDay row to website/docs/sheet/help/built-in-converters.md and the zh-CN counterpart (the page added in #1275 lists each converter family).
  3. The branch is based on an older main (126dde9d) and I don't see CI results on GitHub yet. A rebase will surface the docs page and the new Windows CI matrix; worth pushing once so the checks run.

The rest looks good — leap day --02-29, invalid values (--02-30, month 13, full dates), blank/null pattern fallback, and xlsx/xls/csv round-trips are all covered. LGTM from my side, non-blocking items aside.

@213f08
213f08 force-pushed the feat-monthday-converter branch from ff051ba to b939134 Compare November 10, 2026 13:53
@213f08

213f08 commented Nov 10, 2026

Copy link
Copy Markdown
Author

Thanks for the thorough review, @Mikkey-f — and for the red/green verification, that's very useful for a first-time contributor.

Addressed all three non-blocking items in b939134 (rebased onto current main):

  1. Locale: added DateUtils.parseMonthDay / DateUtils.format(MonthDay, ...) mirroring the parseLocalTime / format(LocalTime, ...) helpers, so the configured GlobalConfiguration locale now flows into the locale-keyed DateTimeFormatter cache. Added a unit test pinning Locale.US vs Locale.FRENCH ("March 15" vs "mars 15", both read back).
  2. Docs: added the MonthDay row to website/docs/sheet/help/built-in-converters.md and the zh-CN counterpart.
  3. Rebase: branch is now based on 29c7dc2 (current main), so the docs page and the Windows CI matrix are in scope.

Local verification on the rebased tree (JDK 17, Windows): fesod-common 60 + fesod-sheet 1,849 tests, 0 failures; spotless:check clean.

The workflows are currently action_required (first-time contributor); could a maintainer approve them so CI can run?

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.

2 participants