Add Date/Time/DateTime utility helpers (draft – interface discussion) by MircoValentiniECMWF · Pull Request #285 · ecmwf/eckit

MircoValentiniECMWF · 2026-03-23T11:17:53Z

Description

This is a draft PR whose primary purpose is to start a discussion on the proposed interfaces for date/time utilities.

The functionalities introduced here are a set of basic manipulation helpers for Date, Time, and DateTime, such as:

shifting (seconds, minutes, hours, days, months, years)
begin/end of day, month, year
simple calendar queries (e.g. leap year, days in month)

These utilities are not new in terms of functionality. They have already been implemented and used in MultIO, in particular for statistics handling. I now need the same capabilities in Metkit, specifically for the encoder.

Given that:

these are generic utilities
they are needed in multiple components (MultIO, Metkit)
they logically belong to the core date/time abstractions

it is more appropriate to centralize them in eckit rather than duplicating implementations across projects.

Scope and Intent

This PR focuses on API introduction, not on final design decisions.
The implementation is intentionally minimal and conservative, aiming to:
- avoid changes to existing APIs
- preserve backward compatibility
- provide a concrete basis for discussion

Notes

Both in-place (mutating) and copy-style (with...) variants are provided to explore usage patterns.
No attempt has been made to fully optimize or refactor the existing classes at this stage.
Naming and exact semantics are open for discussion.

Goal

The goal is to agree on:

the interface shape
naming conventions (add vs shift, beginOf vs alternatives, etc.)
mutating vs non-mutating patterns

before considering any deeper refactoring or optimization.

Contributor Declaration

By opening this pull request, I affirm the following:

All authors agree to the Contributor License Agreement.
The code follows the project's coding standards.
I have performed self-review and added comments where needed.
I have added or updated tests to verify that my changes are effective and functional.
I have run all existing tests and confirmed they pass.

🌦️ >> Documentation << 🌦️
https://sites.ecmwf.int/docs/dev-section/eckit/pull-requests/PR-285

Copilot

Pull request overview

This draft PR introduces a set of date/time utility helpers on the core eckit::Date, eckit::Time, and eckit::DateTime types to centralize commonly needed calendar/time manipulation APIs (shifting and begin/end-of period helpers) for use across projects.

Changes:

Add shift* and withShift* helpers to Time (seconds/minutes/hours) including an optional dayCarry output.
Add calendar query + shifting + begin/end-of-month/year helpers to Date.
Add corresponding query + shifting + begin/end-of-day/month/year helpers to DateTime built on Date and Time.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
`src/eckit/types/Time.h`	Declares new shift and copy-style shift helpers; adds `DateTime` friendship.
`src/eckit/types/Time.cc`	Implements shift logic with day-carry and normalisation.
`src/eckit/types/Date.h`	Declares leap-year/days-in-month queries plus shifting and begin/end helpers.
`src/eckit/types/Date.cc`	Implements month/year shifting (with clamping), begin/end helpers, and calendar queries.
`src/eckit/types/DateTime.h`	Declares new shifting and begin/end-of-period APIs and calendar queries.
`src/eckit/types/DateTime.cc`	Implements DateTime utilities by composing `Date` + `Time` behaviors.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-23T11:22:53Z

src/eckit/types/Time.cc

+    const long day = static_cast<long>(secondsInDay);
+    const long total = std::lround(seconds_) + n;
+
+    dayCarry = computeDayCarry(total, day);
+    seconds_ = normalizeSeconds(total, day);
+


Time::shiftSeconds() currently uses std::lround(seconds_) to compute total, which will mutate the Time by rounding away any fractional seconds even when shifting by 0 (and can also change dayCarry around boundaries, e.g. 86399.6 rounds to 86400). This is observable because Time stores Second as double and DateTime already allows fractional seconds (used for rounding in iso()). Consider computing carry/normalisation using Second total = seconds_ + n and a floor-based carry (and then normalise with fmod/manual adjustment) so fractional seconds are preserved and shiftSeconds(0) is a no-op.

Suggested change

const long day = static_cast<long>(secondsInDay);

const long total = std::lround(seconds_) + n;

dayCarry = computeDayCarry(total, day);

seconds_ = normalizeSeconds(total, day);

// If there is no shift, do not modify the stored time at all.

if (n == 0) {

dayCarry = 0;

return *this;

}

const Second day = secondsInDay;

const Second total = seconds_ + static_cast<Second>(n);

// Compute day carry using floor-based division so negative values are handled correctly.

dayCarry = static_cast<long>(std::floor(total / day));

// Normalise seconds_ into the range [0, day), preserving any fractional component.

seconds_ = total - static_cast<Second>(dayCarry) * day;

// Guard against edge cases due to floating point rounding.

if (seconds_ < 0.0) {

seconds_ += day;

--dayCarry;

}

else if (seconds_ >= day) {

seconds_ -= day;

++dayCarry;

}

Copilot · 2026-03-23T11:22:53Z

src/eckit/types/Time.h

+    friend class DateTime;
+


friend class DateTime; was added but DateTime doesn’t appear to access Time’s private members (it uses the public API). Keeping this friend relationship increases coupling and makes it easier to bypass invariants later; consider removing it unless there is a concrete use-case in this PR.

Suggested change

friend class DateTime;

Copilot · 2026-03-23T11:22:53Z

src/eckit/types/Time.h

+    Time& shiftSeconds(long n);
+    Time& shiftSeconds(long n, long& dayCarry);
+
+    Time& shiftMinutes(long n);
+    Time& shiftMinutes(long n, long& dayCarry);
+
+    Time& shiftHours(long n);
+    Time& shiftHours(long n, long& dayCarry);


New shift APIs (shiftSeconds/Minutes/Hours and withShift* variants) introduce new wrap-around and day-carry behavior but there are no corresponding unit tests validating boundary cases (e.g. 23:59:59 +/- n, negative shifts, and fractional-second inputs used by DateTime). Please add coverage to the existing Time tests to lock down semantics.

Copilot · 2026-03-23T11:22:54Z

src/eckit/types/Date.h

+    bool isLeap() const;
+    long numberOfDaysInMonth() const;
+
+    Date& shiftMonths(long n = 1);
+    Date& shiftYears(long n = 1);
+
+    Date& beginOfMonth();
+    Date& endOfMonth();
+
+    Date& beginOfYear();
+    Date& endOfYear();


Date gains non-trivial calendar logic (leap-year, days-in-month, month/year shifting with day clamping, begin/end of month/year). There doesn’t seem to be direct unit test coverage for Date behavior in the test suite; please add tests for edge cases like shifting from Jan 31 to Feb, leap years (Feb 29), and negative month shifts.

Copilot · 2026-03-23T11:22:54Z

src/eckit/types/DateTime.h

+    bool isLeap() const;
+    long numberOfDaysInMonth() const;
+
+    DateTime& shiftSeconds(long n = 1);
+    DateTime& shiftSeconds(long n, long& dayCarry);
+
+    DateTime& shiftMinutes(long n = 1);
+    DateTime& shiftMinutes(long n, long& dayCarry);
+
+    DateTime& shiftHours(long n = 1);
+    DateTime& shiftHours(long n, long& dayCarry);
+
+    DateTime& shiftDays(long n = 1);
+    DateTime& shiftMonths(long n = 1);
+    DateTime& shiftYears(long n = 1);
+
+    DateTime& beginOfDay();
+    DateTime& endOfDay();
+
+    DateTime& beginOfMonth();
+    DateTime& endOfMonth();
+
+    DateTime& beginOfYear();
+    DateTime& endOfYear();
+


DateTime introduces a fairly large surface of new mutating/copy-style helpers (shift*, begin/end of day/month/year) that can subtly affect date rollover (especially around midnight and month/year boundaries). Please add focused tests for these behaviors (including negative shifts and end-of-month/year transitions) to ensure the API semantics are stable.

codecov-commenter · 2026-03-23T11:48:46Z

Codecov Report

❌ Patch coverage is 0% with 262 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.03%. Comparing base (7743ce5) to head (574d474).

Files with missing lines	Patch %	Lines
src/eckit/types/DateTime.cc	0.00%	131 Missing ⚠️
src/eckit/types/Date.cc	0.00%	71 Missing ⚠️
src/eckit/types/Time.cc	0.00%	60 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #285      +/-   ##
===========================================
- Coverage    66.35%   66.03%   -0.32%     
===========================================
  Files         1126     1126              
  Lines        57668    57930     +262     
  Branches      4403     4405       +2     
===========================================
- Hits         38264    38253      -11     
- Misses       19404    19677     +273

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

simondsmart · 2026-03-23T11:47:15Z

src/eckit/types/Date.cc

+    return shiftMonths(12 * n);
+}
+
+Date& Date::beginOfMonth() {


begin is a verb, not a noun.

You want 'startOfMonth', (and equivalently for other places you have used 'begin' in this file).

Thanks, I fully agree; I am usually really bad in choosing names

simondsmart · 2026-03-23T11:49:23Z

src/eckit/types/DateTime.h

+    bool isLeap() const;
+    long numberOfDaysInMonth() const;
+
+    DateTime& shiftSeconds(long n = 1);


I'm not sure that I like this interface with mutable methods.

Can we rather have something like:

DateTime value;
DateTime newValue = value + DateTime::Seconds(123);
newValue += DateTime::Months(21);

Based on some earlier tests I ran in the statistics component, I observed that DateTime operations already introduce a noticeable performance overhead. This change is a tentative attempt to avoid adding further cost, in particular by reducing the creation of temporary objects. I know this is a bit hacky.

Ozaq · 2026-03-23T12:24:14Z

woops! misclick!

wdeconinck · 2026-03-23T12:58:24Z

Following pure C routines can also serve as a reference, or verification to get identical result:
https://github.com/ecmwf-ifs/fiat/blob/main/src/fiat/util/ec_datetime.c
which includes https://github.com/ecmwf-ifs/fiat/blob/main/src/fiat/util/internal/julian.h

This is what has been used at ECMWF for ages now

Author: Dr. Umberto Modigliani, User Support.
Modified by Willem Deconinck to be more portable using standard integers.

- Originally this file was part of eclib
- Then was ifsaux/eclite/julian.h
- Now part of fiat

wdeconinck · 2026-03-23T13:02:54Z

And did anyone consider to just use C++ std::chrono library? Does that not cater for all needs?

Ozaq · 2026-03-23T17:26:34Z

And did anyone consider to just use C++ std::chrono library? Does that not cater for all needs?

I have the same thoughts, we should use std::chrono and if required provide converters into the exiting types. There seems to be no operation this is not covered by std::chrono.

MircoValentiniECMWF · 2026-03-23T18:34:18Z

Following pure C routines can also serve as a reference, or verification to get identical result: https://github.com/ecmwf-ifs/fiat/blob/main/src/fiat/util/ec_datetime.c which includes https://github.com/ecmwf-ifs/fiat/blob/main/src/fiat/util/internal/julian.h

This is what has been used at ECMWF for ages now
Author: Dr. Umberto Modigliani, User Support.
Modified by Willem Deconinck to be more portable using standard integers.

- Originally this file was part of eclib
- Then was ifsaux/eclite/julian.h
- Now part of fiat

I did not know about this

MircoValentiniECMWF · 2026-03-23T18:44:26Z

And did anyone consider to just use C++ std::chrono library? Does that not cater for all needs?

This is a higher-level design decision that goes beyond the scope of this PR. However, I am happy to explore it further if Simon, Domokos, or Tiago consider it worthwhile.

For context, I previously used std::chrono for this kind of functionality, but I have since switched to eckit::DateTime to remain consistent with the rest of our codebase.

Interface proposal

574d474

MircoValentiniECMWF requested review from Copilot, dsarmany and simondsmart March 23, 2026 11:17

Copilot started reviewing on behalf of MircoValentiniECMWF March 23, 2026 11:18 View session

Copilot AI reviewed Mar 23, 2026

View reviewed changes

MircoValentiniECMWF requested a review from danovaro March 23, 2026 11:25

simondsmart requested changes Mar 23, 2026

View reviewed changes

Ozaq closed this Mar 23, 2026

Ozaq reopened this Mar 23, 2026

MircoValentiniECMWF requested a review from Ozaq March 23, 2026 18:43

-    const long day = static_cast<long>(secondsInDay);
-    const long total = std::lround(seconds_) + n;
-    dayCarry = computeDayCarry(total, day);
-    seconds_ = normalizeSeconds(total, day);
+    // If there is no shift, do not modify the stored time at all.
+    if (n == 0) {
+        dayCarry = 0;
+        return *this;
+    }
+    const Second day   = secondsInDay;
+    const Second total = seconds_ + static_cast<Second>(n);
+    // Compute day carry using floor-based division so negative values are handled correctly.
+    dayCarry = static_cast<long>(std::floor(total / day));
+    // Normalise seconds_ into the range [0, day), preserving any fractional component.
+    seconds_ = total - static_cast<Second>(dayCarry) * day;
+    // Guard against edge cases due to floating point rounding.
+    if (seconds_ < 0.0) {
+        seconds_ += day;
+        --dayCarry;
+    }
+    else if (seconds_ >= day) {
+        seconds_ -= day;
+        ++dayCarry;
+    }

Conversation

MircoValentiniECMWF commented Mar 23, 2026 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Scope and Intent

Notes

Goal

Contributor Declaration

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Mar 23, 2026

Codecov Report

Uh oh!

simondsmart Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

MircoValentiniECMWF Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

simondsmart Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

MircoValentiniECMWF Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Ozaq commented Mar 23, 2026

Uh oh!

wdeconinck commented Mar 23, 2026

Uh oh!

wdeconinck commented Mar 23, 2026

Uh oh!

Ozaq commented Mar 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MircoValentiniECMWF commented Mar 23, 2026

Uh oh!

MircoValentiniECMWF commented Mar 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

MircoValentiniECMWF commented Mar 23, 2026 •

edited by github-actions bot

Loading

Ozaq commented Mar 23, 2026 •

edited

Loading