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

Implement TemporalAdjusters or similar mechanism #20

Open
kagmole opened this issue Feb 13, 2020 · 6 comments
Open

Implement TemporalAdjusters or similar mechanism #20

kagmole opened this issue Feb 13, 2020 · 6 comments

Comments

@kagmole
Copy link
Contributor

kagmole commented Feb 13, 2020

Stop me if I'm wrong, but there is no simple, built-in way to do operations like the following in Carbon:

$nowDate = CarbonImmutable::now();

$previousWeekDate = $nowDate->subWeek();
$startOfPreviousWeekDate = $previousWeekDate ->startOfWeek();

The ThreeTen way is the use of TemporalAdjuster:

LocalDate nowDate = LocalDate.now();

// Custom temporal adjuster
TemporalAdjuster previousWeekAdjuster = t -> t.minus(Period.ofDays(7));

// Static, utility temporal adjusters
TemporalAdjuster firstMondayAdjuster = TemporalAdjusters.previousOrSame(DayOfWeek.MONDAY);

LocalDate previousWeekDate = nowDate.with(previousWeekAdjuster);
LocalDate startOfPreviousWeekDate = previousWeekDate.with(firstMondayAdjuster);

If it is OK, I would like to investigate and if possible (most likely) implement the ThreeTen way.

@BenMorel
Copy link
Member

Hi, you can subtract a week with ->minusWeeks(1), however indeed, there is no way to get the previous Monday.

I'll happily accept a PR for this; however, you don't necessarily have to do it the Java way, you may just add a previousOrSame(int $dayOfWeek) method to LocalDate!

@kagmole
Copy link
Contributor Author

kagmole commented Feb 13, 2020

I try to stick to the ThreeTen API since this is the base of the project. This is to keep the API consistent with other ThreeTen backports like JS Joda as well.

But if you feel this is going to be too cumbersome, let me know.

@BenMorel
Copy link
Member

TBH this library has already deviated a bit from ThreeTen (for better of for worse), and I'm not sure whether I want to continue walking in their footsteps.

For now, I'd accept a PR with only the method in question. If there's a plan to stick to the Threeten API, we can still do it later, there will be a BC break anyway.

I do like the foundation layed by Threeten, however I feel like their API is a bit bloated, and this particular use case "give me the previous monday" is a good example IMO.

What else can TemporalAdjusters do, exactly?

@kagmole
Copy link
Contributor Author

kagmole commented Feb 13, 2020

A TemporalAdjuster is just a lambda (SAM interface). It takes one argument, the object being modified, and returns the modified version of the same type.

Since this is a lambda, you may create a more complex TemporalAdjuster than just "give me monday":

TemporalAdjuster previousMondayAdjuster = (Temporal baseT) -> {
    Temporal previousWeekT = baseT.minus(Period.ofDays(7));
    Temporal previousMondayT = previousWeekT.with(TemporalAdjusters.previousOrSame(DayOfWeek.MONDAY));

    return previousMondayT;
};

The use site then ends up like this:

LocalDate nowDate = LocalDate.now();

LocalDate previousMondayDate = nowDate.with(previousMondayAdjuster);

It is basically a way to allow developers to create custom manipulation "methods". Rather than fixing a set of methods like:

  • previousWeek()
  • startOfWeek()
  • Etc.

You only give one with(..) method that accepts a lambda.

But you got a point since simple operations like "substract one week" do have utility methods (minusWeeks(..)) in addition to the with(..) method. We could add simpler utility method like the one you mentioned or similar, like LocalDate.withDayOfWeek(DayOfWeek.MONDAY).

Note: TemporalAdjuster works on Temporal, which is the base interface for all temporal classes in the original ThreeTen. That is another "pro", it works on any temporal class.

Anyway, I will dig into the subject a little more. I will post additional feedbacks as to "why TemporalAdjuster exists" if I get some. :)

@BenMorel
Copy link
Member

BenMorel commented Feb 13, 2020

This is interesting, although I'm still not sure if I see a value for now. First of all, I can see an issue with returning a generic Temporal: we lose the type-hint in PHP, forcing to type-hint returned values as e.g. /** @var LocalDate $var */ to keep static analysis capabilities, which I don't like.

And, what's the point of creating your own Adjuster:

class FirstMondayAdjuster implements TemporalAdjuster {
    ...
}

$adjuster = new FirstMondayAdjuster();
$adjustedDate = $localDate->with($adjuster);

...when you can just create a class that does adjustments directly?

class Adjuster {
    public function firstMonday(LocalDate $localDate) : LocalDate {
        ...
    }
}

$adjuster = new Adjuster();
$adjustedDate = $adjuster->firstMonday($localDate);

We could add simpler utility method like the one you mentioned or similar, like LocalDate.withDayOfWeek(DayOfWeek.MONDAY).

I'd be happy with previousOrSame($dayOfWeek) and nextOrSame($dayOfWeek), to at least borrow some naming conventions from ThreeTen.

@jurchiks
Copy link

jurchiks commented Sep 27, 2021

I believe he wants the equivalent of https://www.php.net/manual/en/datetime.modify.php except not a string modifier but more advanced. PHP does not support callback type hinting though (yet), so that would be a small problem...
Since this project has a Duration class, you could accept that as an argument instead, the only problem would be the direction (+/-).
It is a shame that this project's Interval class is not the same as PHP's built-in https://www.php.net/manual/en/class.dateinterval.php, because if it was, you could have used that instead.

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