Repository navigation
Conversation
There was a problem hiding this comment.
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:
MonthDayStringConverterTest16/16 +MonthDayConverterDataTest15/15 = 31 - full: fesod-common 60 + fesod-sheet 1,820 = 1,880 tests, 0 failures
spotless:checkok
Red:
- reverting only the
DefaultConverterLoaderregistration (keeping the converter class) makes the registration test NPE and 12 of the 15 round-trip cases fail withCan not find 'Converter' support class MonthDay; restoring turns them green.
Non-blocking
- The configured locale is ignored for
@DateTimeFormatpatterns. The sibling string converters passglobalConfiguration.getLocale()down toDateUtils(YearStringConverter,LocalDateStringConverter,LocalTimeStringConverter), whileDateTimeFormatter.ofPattern(format)here always uses the JVM default locale. Probe (not committed): on a zh_CN JVM, withGlobalConfiguration.locale = Locale.USand@DateTimeFormat("MMMM d"), both writes produced the JVM-default Chinese month name and readingMarch 15threwDateTimeParseException; reading三月 15succeeded only because the default locale happened to match. Numeric patterns such asMM/ddare unaffected. To align, either addDateUtils.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. - Docs: after rebasing onto current main, please add a
MonthDayrow towebsite/docs/sheet/help/built-in-converters.mdand the zh-CN counterpart (the page added in #1275 lists each converter family). - 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.
ff051ba to
b939134
Compare
|
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
Local verification on the rebased tree (JDK 17, Windows): The workflows are currently |
Purpose of the pull request
Related: #1017
Implements the
java.time.MonthDayconverter proposal from #1017, following the existingLocalDate/LocalTimeconverter family pattern. Rebased onto currentmain.What's changed?
org.apache.fesod.sheet.converters.monthday.MonthDayStringConverter:--MM-ddviaDateUtils.parseMonthDay, with a lenientMM-ddfallbackDateUtils.format(MonthDay, ...)@DateTimeFormat(...)when a pattern is configured; an empty ornullpattern falls back to ISO. The configuredGlobalConfigurationlocale is passed toDateUtils, which reuses the locale-keyedDateTimeFormattercache (matchingLocalDateStringConverter/OffsetDateTimeStringConverter)DateUtils.parseMonthDay/DateUtils.format(MonthDay, ...)helpers, mirroring the existingparseLocalTime/format(LocalTime, ...)patternDefaultConverterLoader(all-converter, default-write and default-write-string maps), mirroringInstantStringConverter/YearStringConverterMonthDayrow added to the built-in converters table (en + zh-CN)MonthDayStringConverterTest: ISO and lenientMM-ddparsing; 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.USvsLocale.FRENCH); blank/nullpattern falling back to ISO; boundary values round-trip (--01-01,--02-29,--12-31); converter registration for read and writeMonthDayConverterDataTest: 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 aLocalDatefield in the same rowThis change was prepared with AI assistance and verified locally: full module suite
fesod-common60 +fesod-sheet1849 tests, 0 failures;spotless:checkclean.Checklist