Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Interface for any object representing an interval of time #67

Open
gnutix opened this issue Sep 15, 2022 · 8 comments
Open

Interface for any object representing an interval of time #67

gnutix opened this issue Sep 15, 2022 · 8 comments

Comments

@gnutix
Copy link
Contributor

gnutix commented Sep 15, 2022

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
Copy link
Contributor Author

gnutix commented Oct 9, 2023

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

@BenMorel
Copy link
Member

@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
Copy link
Member

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
Copy link
Contributor Author

gnutix commented Oct 16, 2023

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
Copy link
Member

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
Copy link
Contributor Author

gnutix commented Oct 17, 2023

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
Copy link

👍

@gnutix
Copy link
Contributor Author

gnutix commented Mar 23, 2024

@BenMorel Any thoughts on this matter?

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

No branches or pull requests

3 participants