brick / date-time

Date and time library for PHP
MIT License
334 stars 30 forks source link

Interface for any object representing an interval of time #67

Open gnutix opened 2 years ago

gnutix commented 2 years ago

Hello there,

We've started integrating our code a while ago with this package. In some places, we've extended over time the number of classes that are supported as input by a method.

The "worst example" (in the sense: having the greatest number of supported classes) we currently have looks like this :

public function doSomething(
    LocalDate|LocalDateTime|LocalDateInterval|LocalDateTimeInterval|YearWeek|YearMonth|Year $temporal,
): void;

There's two types of input here :

  1. brick/date-time: LocalDate, LocalDateTime, YearWeek, YearMonth, Year
  2. gammadia/date-time-extra: LocalDateInterval, LocalDateTimeInterval

Most of the time, the first thing done in such a method will be something like :

$temporal = LocalDateTimeInterval::cast($temporal);

As everyone of these objects can be represented with a LocalDateTimeInterval, which is the most "precise" of all. This allows to then use complex methods like intersects and such, which are not necessarily implemented on each of these objects.

It would be nice if the signature of the method could simply be :

public function doSomething(Temporal $temporal): void;

(or TemporalInterval, or some other name that would represent "any time interval").

Just having an empty* interface implemented on Brick's classes would allow that. Is that something you might be open to ? I'm very reluctant to make a fork just for a such a simple addition, and not keen on having such complex union types either... (without taking about validation, error messages, etc... everything gets more complex)

I'll gladly submit a PR if you're OK with the idea.

Thanks for your consideration. gnutix

*: it could also contain some methods / extend other interfaces you're definitely sure you want to have on all these objects, like Stringable or JsonSerializable, or even go as far as proposing methods to get time boundaries (think getStart / getEnd) or compare methods (isEqualTo, isBefore, isAfter...) - though it might get complex to get types right...

gnutix commented 12 months ago

@BenMorel Have you had a chance to think about this ?

BenMorel commented 11 months ago

@gnutix Sorry for the delay, I actually have a use case for a Temporal interface, implemented by all value objects, that could be used as a basis for #61. I'll probably work on this in the coming days (I need to figure out what to cherry-pick from Java's temporal API first).

BenMorel commented 11 months ago

Note: I'm not sure how this interface could apply to intervals, though? The interface's signature might look like:

interface Temporal
{
    public function get(TemporalField $field): int;
}

With TemporalField an enum with values such as YEAR, MONTH_OF_YEAR, DAY_OF_MONTH, etc.

(see the TemporalAccessor interface and the ChronoField enum in Java)

How can this work with intervals?

By the way, to better understand, can I know your original use case for doSomething(LocalDate|LocalDateTime|LocalDateInterval|LocalDateTimeInterval|YearWeek|YearMonth|Year $temporal)? The concepts of each accepted class look vastly different, so I'd be interested in knowing a bit more.

gnutix commented 11 months ago

Here's a few examples :

interface LocalDateTimeInterval {
    public static function cast(null|string|self|LocalDate|LocalDateTime|LocalDateInterval|YearWeek|YearMonth|Year $temporal): ?self;

    public static function containerOf(null|self|LocalDate|LocalDateTime|LocalDateInterval|YearWeek|YearMonth|Year ...$temporals): ?self;

    public function precedes(self|LocalDate|LocalDateTime|LocalDateInterval|YearWeek|YearMonth|Year $temporal): bool;
    public function precededBy(self|LocalDate|LocalDateTime|LocalDateInterval|YearWeek|YearMonth|Year $temporal): bool;

    // ... repeats for all ALLEN relations
}

interface TemporalHelper {
    public static function parse(?string $temporalIso): null|LocalDate|LocalDateTime|LocalDateInterval|LocalDateTimeInterval|YearWeek|YearMonth|Year;

    public static function isOvernight(null|LocalDate|LocalDateTime|LocalDateInterval|LocalDateTimeInterval|YearWeek|YearMonth|Year ...$temporals): bool;
    public static function intersects(null|LocalDate|LocalDateTime|LocalDateInterval|LocalDateTimeInterval|YearWeek|YearMonth|Year ...$temporals): bool;
}

I don't think there can be any method on this Temporal interface. Maybe sub-interfaces could contain some ?

The point of these signatures is to simplify code at call-site, where you don't have to care about what type of temporal data you're manipulating anymore : you can directly compare (with any of the methods given above) a LocalDateInterval with a YearMonth object (for example), and internally, both would be converted to LocalDateTimeInterval objects before doing the comparison.

So sure, for some of the combinations, it would make no sense. Like comparing a LocalDate with a Year ; it would never match. But still, the code allows to do so and would simply return false (which is the expected behavior).

BenMorel commented 11 months ago

Thank you for clarifying this, @gnutix.

We could have a base Temporal interface with no methods, and sub-interfaces such as TemporalAccessor, but we would slightly deviate from Java, where Temporal has methods for adding/subtracting temporal units as well.

I'm not planning on adding a generic method for adding/subtracting units in Temporal for now so this is not a problem for you yet; but if we did add them to Temporal at some point, would there be a possible implementation for your custom date/time classes?

gnutix commented 11 months ago

I'm not entirely sure what "adding/subtracting units" means (I've not looked at the Java implementation set, sorry), but I can tell you we already have a "move" method that takes a Duration/Period and uses it to add it to the interval (ex: 2023-10-17/2023-10-17 ->move(Period::ofDays(1)) === 2023-10-18/2023-10-18). So it should be okay.

maxisusi commented 10 months ago

:+1:

gnutix commented 6 months ago

@BenMorel Any thoughts on this matter?