[Discussion] Where should `isDate1904` and `getDateTimeFormat` reside? (`DateUtils` vs. `AbstractDateTimeConverter`)

[bengbengbalabalabeng]

Background & Current State

In several recent PRs introducing or improving date/time converters (e.g., SqlTime*, Year*, OffsetDateTime*, SqlDate*, etc.), we noticed that there is some duplicated boilerplate code, along with inconsistencies in edge-case handling:

1. Inconsistent format extraction and empty string handling

• Some implementations check StringUtils.isEmpty(format) and normalize it to null, while others pass empty strings directly downstream, which can lead to unexpected parsing errors.

• The format extraction logic is repeatedly implemented across different converters:

  String format = null;
  if (contentProperty != null && contentProperty.getDateTimeFormatProperty() != null) {
      format = contentProperty.getDateTimeFormatProperty().getFormat();
  }

2. Layering issue with 1904 windowing check

Currently, DateUtils contains the following method:

public static boolean isDate1904(ExcelContentProperty contentProperty, GlobalConfiguration globalConfiguration)

This causes the low-level, general-purpose date utility class to directly depend on framework-level metadata models (ExcelContentProperty and GlobalConfiguration), introducing architectural coupling.

---

Proposed Options

To standardize the implementation for future date/time types, eliminate boilerplate code, and unify edge-case behaviors, I suggest we tidy up this bit of logic. Two main directions are being considered:

Option A: Centralize into DateUtils (Keep the status quo)

Add getDateTimeFormat(ExcelContentProperty) to DateUtils, keeping it at the same level as the existing isDate1904.

• Pros: Minimal changes required; no new class hierarchy introduced; keeps the codebase flat.
• Cons: The low-level utility class continues to take on framework-level metadata resolution, causing its responsibilities to bloat further.

Option B: Introduce an abstract base class AbstractDateTimeConverter<T>

• Encapsulate getDateFormat(ExcelContentProperty) and isDate1904(ExcelContentProperty, GlobalConfiguration) within the base class.

• Since DateUtils.isDate1904 was introduced recently and has not yet been included in any official release, it can be directly deprecated or removed without backwards-compatibility baggage. (already in the 2.1.0 release candidate)

• Concrete converter subclasses can focus solely on core data conversion logic.

• Pros:

  • Unifies edge-case behaviors.
  • Keeps metadata resolution visible only to date/time converters, preserving the purity and single responsibility of DateUtils.

• Cons: Introduces a new abstraction layer and inheritance hierarchy.

---

Feedback Welcome

Which option do you prefer? Or do you have alternative suggestions? Feel free to share your thoughts below!

• +1 [Option A / Option B] {additional information - optional}
• -1 [Option A / Option B] {reason}
• +0 [Alternative proposals or thoughts]

[Mikkey-f]

+0 on A vs B, with one timing fact and a third option.

A fact that may affect the choice: isDate1904 is already in the 2.1.0 release candidates

2.1.0-incubating-rc5 and rc6 already contain DateUtils.isDate1904(...), and five read-side number/string converters call it. So "not yet included in any official release" holds only until 2.1.0 ships from one of those candidates — after that, removing it from DateUtils is a breaking change to released API and would need a deprecation cycle, unless the release is re-cut from a later main. Worth confirming with the release manager before we treat the removal as free.

Option C: keep the metadata-aware helpers in the converter layer

Instead of growing DateUtils (A) or introducing an inheritance level (B), the helpers could live in the converter layer — a small static helper next to the date converters — leaving DateUtils with pure date/string primitives. It addresses the same layering concern as B, without changing the class hierarchy of the whole date/time family, and keeps the diff small.

One detail worth weighing for B: the format extraction is needed on both the read and write side, but the 1904 check is only used by the number/string read converters, so that part of a shared base class would go unused by most subclasses. (And if the removal window has closed by then, DateUtils.isDate1904 would remain as a deprecated delegate, which leaves the layering coupling in place either way.)

Sequencing

These call sites are the same files #1033 (the OffsetDateTime converters, which I authored) is touching right now, and it adds one more format-extraction site of its own. Whatever we settle on, landing the cleanup after #1033 merges would be cheaper than having both edit the same files — I am happy to help migrate the family, including the OffsetDateTime ones, once the direction is agreed.

[bengbengbalabalabeng]

A fact that may affect the choice: isDate1904 is already in the 2.1.0 release candidates

Thanks for the clarification; I've updated the description.