spatie / period

Complex period comparisons
https://spatie.be/open-source
MIT License
1.6k stars 72 forks source link

[2.0] Configure Date class to use #44

Closed kylekatarnls closed 5 years ago

kylekatarnls commented 5 years ago

Hi guys,

Periods are always immutable, there's never the worry about your input dates being changed.

This seems a bit too much opinionated to me for a Period library.

I think it should be an agnostic DateTimeInterface in typings (so stay flexible with inheritance ing PHP < 7.4) as in https://github.com/spatie/opening-hours

And I can propose a PR to have a class customization like in CarbonPeriod: https://github.com/briannesbitt/Carbon/blob/master/src/Carbon/CarbonPeriod.php#L228

It means you would have DateTimeImmutable as the default class, but other classes could be set as long as they implements DateTimeInterface so users could choose if getStart() and similar methods would return DateTimeImmutable, DateTime, Carbon, Chronos etc. objects.

CarbonPeriod has both an IMMUTABLE flag option to switch Carbon/CarbonImmutable and a ->setDateClass() method to switch to any DateTimeInterface sub-class. And any class that extends CarbonPeriod can easily change the default class by overriding the property default value.

Are you interested in a similar way to choose the date class for Spatie\Period\Period?

brendt commented 5 years ago

Hi Kyle

It was a deliberate design design to enforce immutable dates. The package provides mutable support via the ::make method, though as soon as the period is constructed, they are always immutable.

Having been the "end user" of mutable and immutable datetime packages for years now, I personally don't see any benefit of allowing mutable datetimes. Though if you're able to give good arguments why mutable dateimes should be allowed, I'm open to hear them.

To further support my opinion: when working with dates and periods in my client projects, I prefer strictness and certainty over flexibility. I'm building invoice systems that require absolute correctness, I can't afford to have bugs because of a mutable datetime being changed somewhere without me knowing. People who are looking for flexibility can use other libraries which better fit their needs, but I don't think we'll allow mutable internal dates anytime soon.


Moving on the the second issue, this might be more on topic with the actual goal of your question.

I do agree that being able to return any immutable datetime object from eg. getStart would be a good feature for this library. But I don't think allowing all kinds of datetime implementations within the Period is the right way to go here. I'd prefer to wait for covariant return type support in PHP 7.4 to allow this:

class CustomPeriod extends Period
{
    public function getStart(): Carbon
    {
        return Carbon::make($this->start);
    }
}

Again, feel free to share your thoughts on this. I always appreciate people who help improve this library, and I'm open to hear counter arguments to mine 😊

kylekatarnls commented 5 years ago

I fully agree immutable should be the default. As you can see, I do my best to help people use immutable dates: https://github.com/laravel/framework/pull/25320

But as spatie/period does not modify the dates itself, it sounds to me like the library oversteps its role.

If every library enforces a typing (in places they could easily accept a wider one), then you get library A enforcing Chronos dates, library B enforcing DateTimeImmutable, and an old Laravel that only supports Carbon (the mutable one), then you will have a lot of pain with connecting them all, and will have conversions everywhere.

While if all those libraries were more agnostic, as soon as the default behavior does not fit your stack, you could just tell the library: this is the date class I use, please work with it, I undertake the consequences.

I'd prefer to wait for covariant return type support in PHP 7.4

I exactly thought this would be the perfect use-case for them.

If spatie/period would have a lot of ->modify() calls, I would understand you couldn't trust them. But dates in this library are in fact just input/output, so it would stick to the "period" responsibility to allow any type of date.

brendt commented 5 years ago

I still find this a common use case though:

$period = new Period($mutableStart, $mutableEnd);

if ($period->getStart()->addMonth() > $input)
{

}

Technically this is not this package's responsibility, but the developer is now concerned with the question whether he used the correct immutable date. Mind you, there are often cases where a period is not constructed right before doing these kinds of checks, making it even harder.

With our current approach, I can always be 100% sure nothing can go wrong if I modify the dates, I'd even call that one of the features this package provides. For sure it was one of the main reasons I made it to start with.

then you get library A enforcing Chronos dates, library B enforcing DateTimeImmutable, and an old Laravel that only supports Carbon

I don't think this is a problem because of the ::make method: all of these inputs are supported. They just aren't used once inside the Period itself.

What we could consider is adding a MutablePeriod implementation. I realise though that this approach is opposite from almost every other datetime implementation where mutable is the default: DateTime vs DateTimeImmutable and Carbon vs CarbonImmutable.

I simply think immutable by default has way too much advantages over mutables. I believe we are in agreement over that immutable should be the default.

I'm still open for your input! Feel free to try and change my mind 😊

kylekatarnls commented 5 years ago

I can always be 100% sure nothing can go wrong if I modify the dates

It's because you expect immutable dates, put yourself in the guy who has always worked with mutable dates shoes.

$period = new Period(new DateTime('now'), new DateTime('next Friday'));
$start = $period->getStart(); // Here you intuitively think you have new DateTime('now')
$start->modify('+1 month'); // And so you manipulate it like you're used to
// Later
echo $start;

Dammit, I gave you a mutable date, I said "modify" (yeah, very misleading name regarding to DateTimeImmutable), and $start is not modified.

Then, the user may not be like us working on date-related libraries. He might not even now anything about DateTimeImmutable. Here he will have to find this is the consequence of "Periods are always immutable" and learn about DateTimeImmutable to work with it.

The same will happen with dates from the iteration. And note than if you get the DatePeriod with getIterator, the DatePeriod object itself is mutable, dates from it will not be.

And still you can convert them by default but add a setting to choose the output format: https://github.com/spatie/period/pull/48/files#diff-72500a76e9b0a3ca287c7e5a08d3de35R43

I don't say the library should use the class of input dates, as start and end can be different. I would rather say it's an option: output class. So the user obviously know what he's doing as he added a argument and will still have DateImmutable no matter what is the input by default. But the presence of the argument make it a bit more obvious dates are transformed.

I just feel more confortable with a "If you don't know, we'll convert this into DateTimeImmutable. But if you don't want it, just tell me." rather than "We know what is good for you, let's get rid of this DateTime sh** you gave me."

That's why I chose to re-convert according to user option in the adapter: https://github.com/kylekatarnls/enhanced-period/blob/master/src/Cmixin/EnhancedPeriod.php#L216 https://github.com/kylekatarnls/enhanced-period/blob/master/tests/Cmixin/EnhancedPeriodTest.php#L284

When doing this, I was thinking it would be cool if the spatie/period conversion was optional, so we would not have here the double-conversion.

I do not prefer mutable dates, but there users who do, and who probably use spatie/period like this:

$period = new Period($mutableStart, $mutableEnd);

foreach ($period as $date)
{
    $mutableDate = DateTime::createFromImmutable($date);
    changeTime($mutableDate);
    echo $mutableDate->format('d H:i');
}

They need to re-convert manually to avoid rewriting functions/libraries that actually modify dates rather than returning new ones. It would be way easier for them to specify the class only once:

$period = new Period($mutableStart, $mutableEnd, null, null, null, DateTime::class);

foreach ($period as $date)
{
    changeTime($date);
    echo $date->format('d H:i');
}

I can propose something else:

// Allow any DateTimeInterface sub-class
abstract class AbstractPeriod implements IteratorAggregate
{
    protected $dateClass = DateTimeImmutable::class;

    protected static function resolveDate($date, ?string $format): DateTimeInterface;
}

// Allow any DateTimeImmutable sub-class
class Period extends AbstractPeriod
{
    protected static function resolveDate($date, ?string $format): DateTimeImmutable;
}

// Allow any DateTime sub-class
class PeriodMutable extends AbstractPeriod
{
    protected $dateClass = DateTime::class;

    protected static function resolveDate($date, ?string $format): DateTime ;
}

This way, you will benefit of a proper IDE auto-completion as Period will have DateTimeImmutable type hint everywhere. But it internally relies on a base-class that can handle all DateTimeInterface so you can extend it to use DateTime without requiring PHP 7.4 covariant types. The PeriodMutable does not even has to be in the library if you want to discourage use of mutable dates. But at least you would not block such extensions.

brendt commented 5 years ago

Thanks for this input, I appreciate it! I think it's time to also ping @jeromegamez and ask his opinion on this.

On to my answer.

but there users who do, and who probably use spatie/period like this

I'd say that this package is simply not the solution for these users. As it's not some hidden feature tucked away, I assume most users know this package only works with immutable dates. It's said so at the start of the README:

Periods are always immutable, there's never the worry about your input dates being changed.

So the question becomes whether we want to maintain a part of this package which I β€” personally β€” don't think should every be used.

I do like your last proposal though. It keeps the mutable cruft separate, and introduces a common interface/abstract class. I actually played around with the idea of adding an interface for periods before, in order to support "endless" periods (that's unrelated to this issue, I just wanted to say that an abstraction probably will add value overall).

As you pointed out, PHP's 7.4 type variance allows for more flexibility, and I think is worth looking further into.

So while I personally don't see any added value in adding it, let's take a look at what JΓ©rΓ΄me thinks of it, and take it from there.

jeromegamez commented 5 years ago

I'd like to start with that I'm not looking for heat similar to the recent "Should classes be final" elsewhere, this is just my personal opinion in the hope that it can be useful πŸ˜….

Also, as this is a 2.0 subject, I feel comfortable talking about some more breaking changes.

I (would like to) see a Period as an immutable value object that serves exactly the purpose that it's advertised with: Complex period comparisons.

I know and appreciate that you are both advocates for being open for extension and making it easy for developers to make changes so that it better fits their individual use cases - but in this case, I'd like to argue that here, the job is not to provide extensibility or flexibility, but to provide reliable and reproducible results when comparing the period with dates or other periods.

The internal state of the Period doesn't matter to the outside world and should continue to use DateTimeImmutables, and Period::make() already accepts anything that can be parsed to a DateTime* and all comparison methods accept any DateTimeInterface, so in my opinion, the Period is as open as it can be.

I would argue that adding support for custom Output Types is not the main concern of a Period and adding an AbstractPeriod and Extensions of that would add complexity in the implementation that doesn't help the main purpose (and to be honest: there's much I don't fully grasp already πŸ˜…).

Okay, enough of "I wouldn't", here are some things I would do:

  1. Make Period::__construct() private and only use the named make() constructor to create new instances. This would enable us to change the internal instantiation without breaking the public one, and we can add as many additional named constructors as we want.
    • This would at least mitigate the "We know what is good for you, let's get rid of this DateTime sh** you gave me." concern.
  2. Perhaps introduce a PeriodBehaviour (I struggle with naming) interface with (a final) Period as its default implementation and a CarbonPeriod as an example custom implementation that wraps the default one and converts the return values to Carbons and CarbonPeriods.
  3. Rethink if we need the Duration and Iterator returning a DatePeriod in the Period at all (I know I introduced them in the first place πŸ˜…, but both have already shown not to be without issues).
    • I know I introduced the duration part, but when I think about the "complex period comparisons", a public duration() query method doesn't add to that. Methods like hasLongerDurationThan($other), hasShorterDurationThan($other), hasSameDurationAs($other) would perhaps do it.
    • Likewise, iterating over a Period is not part of the core job. asDatePeriod() could perhaps be an alternative.

Again, this is only my opinion/what I would probably pursue - to explain where I'm coming from: for me personally, good DX is not only about freedom, but also about clarity and unambiguity and I've made better experiences in the long run with pursuing the latter, and I shared this in the hope to add to the conversation, not to impose 🌺

kylekatarnls commented 5 years ago

the purpose that it's advertised with: Complex period comparisons

If you want to stick with this, then getStart, getIncludedStart, getIterator, etc. should not be public as they return values you say that are internal. Indeed if you remove all those methods, any date will never be output and the DateTimeImmutable you created internally are really private forever. Same for asDatePeriod it's not related to comparison.

1/2/3: I see, and it's more than breaking change, more like paradigm change. Hopefully framework and languages do not have so radical changes in major upgrades. I find the final very... ambitious. Sounds like: don't extend me, I'm perfect like this.

Private constructor, hem, I will have to review my wrapper: https://github.com/kylekatarnls/enhanced-period/blob/master/src/Cmixin/EnhancedPeriod.php#L42

I understand. But I guess many of the version 1 users have intuitively used the constructor as I did. All native classes of PHP have their constructor public. It's something we're very used to in PHP. It could be a big brake for the upgrade. As it would be with the removal of IteratorAggegate

All that sounds like a bright cool new library, but it's really not how I imagined the new spatie/period.

I hope we could have a transition easing for that (deprecation notices in 1.5), alpha/beta releases to let dependent libraries time to get compatible.

jeromegamez commented 5 years ago

Again, I was just sharing my thoughts, not saying that I wanted to get my way.

Let me elaborate on some points that I didn't convey well enough:


Private constructor / Period::make()

There is currently no functional difference between

$period = new Period($start, $end, $precision, $boundary);
$period = Period::make($start, $end, $precision, $boundary);

except for that Period::make() accepts even strings as date-time values.

The way to migrate this from 1.x to 2.x would be to mark the Constructor as @internal in 1.x and point to the named constructor.

Possible implementation ```php /** * @internal * * Access to the constructor will be restricted in 2.0 * * Use {@link make()} instead * * @see make() */ public function __construct(/* ... */) { // ... } ``` ![image](https://user-images.githubusercontent.com/67554/62496727-1edc0500-b7da-11e9-9df0-7ed88bafa7f1.png) ![image](https://user-images.githubusercontent.com/67554/62496799-5d71bf80-b7da-11e9-88fe-6b8b9678c308.png) And then, in 2.0 ```php private function __construct(/* ... */) { // .. } ```

Wait until I tell you that I often make my constructors not only private but also empty πŸ˜….

All native classes of PHP have their constructor public. It's something we're very used to in PHP.

I'm a fellow PHP developer 🌺, and I'm used to them as well, but I'm equally used to named constructors, because most of the classes I work with are not native and static methods are not exactly a novelty ^^.


Named constructors

While writing this reply, use cases for additional named constructors came into my mind:

public function fromDatePeriod(DatePeriod $period): self
{
    $start = $period->getStartDate();
    $end = $period->getEndDate();
    $boundaries = $period->include_start_date 
        ? Boundaries::EXCLUDE_NONE
        : Boundaries::EXCLUDE_START;
    $precision = self::dateIntervalToPrecision($period->getDateInterval());

    return self::make($start, $end, $precision, $boundaries);
}

public static function withDayPrecision($start, $end, ?int $boundaryExclusionMask = null, ?string $format = null): self
{
    return self::make($start, $end, Precision::DAY, $boundaryExclusionMask, $format);
}

public static function withMinutePrecision($start, $end, ?int $boundaryExclusionMask = null, ?string $format = null): self
{
    return self::make($start, $end, Precision::MINUTE, $boundaryExclusionMask, $format);
}

It's, of course, debatable if this makes sense as it is, but it would help users by not needing to care about constants when making a new instance, and I meant it as an example for multiple named constructors.


Internal state vs public accessors

JΓ©rΓ΄me: the purpose that it's advertised with: Complex period comparisons

Kyle: If you want to stick with this, then getStart, getIncludedStart, getIterator, etc. should not be public as they return values you say that are internal.

I disagree.

There's a difference between parameters to create a period, the internal state and public properties.

When you create a Period you give it a start date and an end date as parameters.

Internally, these parameters are processed and stored for internal usage.

When you use Period::getStart(), you don't get the start date parameter, you get the start of the period. $startDate, $this->start and getStart() have the same nomenclature, happen to be the same value, but have different contexts. $startDate is a self-contained value, getStart() is the start of a period.

From my point of view, the Period could be more clear on what it will provide, and I find the differentiation between Period::getStart() and Period::getIncludedStart() confusing, so it could be discussed whether the Period should even care about included and excluded boundaries, except when processing the parameters.

On the one hand, the current implementation gives more information on how a period was created.

On the other hand, we could also say: when creating a period, the boundaries are important, but a Period always has a start and an end, and both are always excluded. For example:

$a = Period::make('2019-08-05', '2019-08-11', Precision::DAY, Boundaries::EXCLUDE_NONE);
$a->getStart(); // 2019-08-05
$a->getEnd(); // 2019-08-11

$b = Period::make('2019-08-05', '2019-08-11', Precision::DAY, Boundaries::EXCLUDE_END);
$b->getStart(); // 2019-08-05
$b->getEnd(); // 2019-08-10

Once a Period is created, we don't care about start vs. included start - a period has a start and one start only, so we could get rid of getIncludedEnd().


What it is vs. what it does

the purpose that it's advertised with: Complex period comparisons

If you want to stick with this, then getStart, getIncludedStart, getIterator, etc. should not be public as they return values you say that are internal. Indeed if you remove all those methods, any date will never be output and the DateTimeImmutable you created internally are really private forever. Same for asDatePeriod it's not related to comparison.

What Period does is to provide means to make comparisons with other Periods and DateTimes. That doesn't mean that it doesn't have properties that it can expose, but I agree that it only should expose the following properties (=properties needed to create a new Period):

The more I think about, the more I am convinced that we should remove getIterator() again, and here's why:

The period is concerned with its properties and comparing itself to others, not with being traversed externally. If we want to give developers the means to iterate over a period, Spatie's period is not the one, but it could help by providing a convenience asDatePeriod(): DatePeriod class.

One might say: "'tomaito tomahto', it doesn't matter, whether it's named "getIterator()" or "asDatePeriod()" - in the end, it returns the same thing", but actually it's not. asDatePeriod() is a convenience method, the reason getIterator() exists is because Period implement the IteratorAggregate interface (which I introduced and now consider a mistake).


Final vs. perfect

I find the final very... ambitious. Sounds like: don't extend me, I'm perfect like this.

Subtle :). Here's the thing: I proposed final in the context of using an Interface.

My personal view: The interface is the contract, the class is the implementation. When I fully implement the contract (thus finalizing it), it's done. If something doesn't work as per contract, it's a bug, and it's the maintainer's job to fix.

My point is not to argue that one view is necessarily better than the other, and I honestly don't care if final is used here or elsewhere. The point "nobody in their right mind would willfully extend a class and break it" is absolutely valid.

But as I accept people's preference for abstraction and inheritance, I would appreciate if the same people would accept that others prefer composition, without assuming that they're arrogant or elitist :) (a recurring subject on social media plattforms, as we all know).


Intuition vs. Expectation

It's because you expect immutable dates, put yourself in the guy who has always worked with mutable dates shoes.

$period = new Period(new DateTime('now'), new DateTime('next Friday'));
$start = $period->getStart(); // Here you intuitively think you have new DateTime('now')
$start->modify('+1 month'); // And so you manipulate it like you're used to
// Later
echo $start;

Dammit, I gave you a mutable date, I said "modify" (yeah, very misleading name regarding to DateTimeImmutable), and $start is not modified.

The nice thing about type hints is that one doesn't have to rely on their intuition when using methods with type hinted parameters and return values.

On the one hand, they can be more restrictive than one might like, for example when you could provide '1' or 1, but an int $var forces you to typecast the value, although PHP would treat both equally... except when, in a future update, it doesn't anymore. On the other hand, they can ensure a reduced amount of surprises and side effects.


Conclusion

I don't want to convince you that "my way" is the right way and that this is how this library should move forward (besides, it's not up to me anyway).

I totally understand the merits of keeping Period open and making it even more accessible by providing an abstract class and making the returned types configurable - it makes it more flexible and easier to implement by third parties like the EnhancedPeriod mixin, and will increase the adaption and popularity of this library, because these are features that are widely appreciated (and the success of Carbon and all the Spatie libraries are proof).

But when I'm asked for my opinion, I give it, in good faith, and I try to underpin it the best I can. I like to use opportunities like this one to check if I can defend my general views (not just to others but also myself) - and I don't claim to be right or that my opinion is shared.

This turned out to be more of an exercise for myself πŸ˜… - sorry if this didn't provide value to the topic in particular or to the reader in general πŸ™ˆ

kylekatarnls commented 5 years ago

Once a Period is created, we don't care about start vs. included start - a period has a start and one start only, so we could get rid of getIncludedEnd().

Exactly. I was thinking of that too. DatePeriod needs to keep the flag because you can change the DateInterval and the end inside. So the exclusion need to be recalculated. But with the immutable position of Period you can calculate start/end, then forgot the exclusion flags and original start/end.

asDatePeriod and fromDatePeriod could a good standard input/output for other library to plug (such as EnhancedPeriod mixin).

In this plan, I identified other potential deprecation to add: getIncludedStart/End getIterator and its interface (need to check how IDEs understand a foreach-able deprecation).

But this plan seems consistent.

kylekatarnls commented 5 years ago

Hi, I checked how it would work for my mixin with the more strict scenario (private constructor, final, no more IteratorAggregator so I'm probably 2.0-ready with this simple change: https://github.com/kylekatarnls/enhanced-period/commit/dfee103223f9533a4879fe94bbd7cd4dafb93ce3

After, rethinking the DatePeriod stuff does not benefit to the CarbonPeriod <=> Spatie\Period as we loose the EXCLUDE_END that both handle property but not DatePeriod (where it is always excluded).

So the following is still better than using the DatePeriod as exchange format:

new CarbonPeriod(
    $period->getStart(),
    $period->getEnd(),
    $period->interval, // is not public right now, had to be recalculated
    ($mutable ? 0 : CarbonPeriod::IMMUTABLE) |
    ($period->startExcluded() ? CarbonPeriod::EXCLUDE_START_DATE : 0) |
    ($period->endExcluded() ? CarbonPeriod::EXCLUDE_END_DATE : 0)
);

Note: I don't think precisionMask should have a public getter while interval hasn't.

But if both become private in 2.0, I still can use ->asDatePeriod()->interval.

brendt commented 5 years ago

when creating a period, the boundaries are important, but a Period always has a start and an end, and both are always excluded

We already had a lengthy discussing on whether we want to support both included and excluded boundaries, and the answer is yes: https://github.com/spatie/period/issues/9

The period is concerned with its properties and comparing itself to others, not with being traversed externally. If we want to give developers the means to iterate over a period, Spatie's period is not the one

I don't agree on this one. I think being able to traverse a period based on its precision is a feature of this package that we shouldn't throw away.

Also while I appreciate all the ideas listed here, we're talking about way more than what the original issue tried to address. Feel free to make new issues with ideas.

So back on topic: allowing mutable periods or not. @kylekatarnls am I correct in assuming that this question originated from your work on enhanced-period? Maybe we should start at what we're trying to achieve exactly? I believe you want to use the period comparisons provided by this package within CarbonPeriod. Is that correct?

I glanced at the source code and from what I can tell, you're using the package as intended, it's wrapped by another class, which uses Period internally.

What problem would be solved if we allowed mutable dates to be used internally in Period?

jeromegamez commented 5 years ago

Uh, the discussion in #9 was before my time, sorry I missed that! And you're right, we digressed quite a bit πŸ˜…

brendt commented 5 years ago

No problem πŸ‘

jeromegamez commented 5 years ago

What problem would be solved if we allowed mutable dates to be used internally in Period?

I believe the internally used types are not the issue, only what the public methods return. Currently, they are type hinted to DateTimeImmutable and e.g. Carbon would need converters in both directions to provide its users with the same type they used to make a Spatie Period.

But Iβ€˜m not sure - in my opinion it would solve a problem/add convenience for libraries using spatie/period, but not for the library itself and instead complicate it.

brendt commented 5 years ago

That shouldn't be a problem though, ImmutableDateTime is a subtype of DateTime, hence everywhere where DateTime is a required type, ImmutableDateTime can also be passed.

jeromegamez commented 5 years ago

Unfortunatly, it's not, both implement DateTimeInterface, but DateTimeImmutable doesn't extend DateTime :/

function test(DateTime $dt) {}

test(new DateTimeImmutable());

https://3v4l.org/4GSjI

Or did I misunderstand what you mean?

kylekatarnls commented 5 years ago

That shouldn't be a problem though, ImmutableDateTime is a subtype of DateTime, hence everywhere where DateTime is a required type, ImmutableDateTime can also be passed.

What is ImmutableDateTime? I only know DateTime and DateTimeImmutable and there is no subtype link between both.

The issue I have in my mixin, but can happen in other context is for example: I have dates, I need start and end dates of the result of let's say gap:

// My input:
$mon = new DateTime('Monday');
$tue = new DateTime('Tuesday');
$wed = new DateTime('Wednesday');
$thu = new DateTime('Thursday');

// Transformation: user-code/third party library
function getGap($a, $b, $c, $d) {
  $gap = Period::make($a, $b)->gap(Period::make($c, $d));

  return [$gap->getStart(), $gap->getEnd()];
}

// My output is 2 DateTimeImmutable objects
[$newTue, $newWed] = getGap($mon, $tue, $wed, $thu);

I found interesting having a way to customize the type of the output in start/end/iteration.

brendt commented 5 years ago

What is ImmutableDateTime? I only know DateTime and DateTimeImmutable and there is no subtype link between both.

I wasn't looking at the code and mistyped, my bad!

// My output is 2 DateTimeImmutable objects

Since Spatie\Period\Periods is used within the context of your enhanced period class, can't you convert the output there to a mutable or immutable date depending or user config? Or is this the thing you want to fix?

kylekatarnls commented 5 years ago

That's what I do, not a problem, I check my input, then convert immutables dates to mutable dates in the output.

Something like this:

// My input:
$mon = new DateTime('Monday');
$tue = new DateTime('Tuesday');
$wed = new DateTime('Wednesday');
$thu = new DateTime('Thursday');

// Transformation: user-code/third party library
function getGap($a, $b, $c, $d) {
  $preferImmutable = $a instanceof DateTimeImmutable;
  $gap = Period::make($a, $b)->gap(Period::make($c, $d));
  $result = [$gap->getStart(), $gap->getEnd()];

  if (!$preferImmutable) {
    $result = array_map($result, [DateTime::class, 'createFromImmutable']);
  }

  return $result;
}

// My output is now 2 DateTime objects
[$newTue, $newWed] = getGap($mon, $tue, $wed, $thu);

But as you can see, this mean double conversion, and additional checking to do each time you get something out of the period. The point of the configuration was to say once for all for a Period what should comes out.

I also found interesting that when using the converter CarbonPeriod::toEnhancedPeriod() so you get a Period and currently you "loose" the mutable/immutable option in the conversion.

An other way is rather than create new object from scratch in roundDate, you could use ->modify() (or clone + ->modify() if mutable) so you preserve the original type and the metadata (for example, the language locale in CarbonImmutable).

But it all just options and I understand it's a layer of complexity. Feel free to close if you think it does not worth the benefit.

brendt commented 5 years ago

Well since the enhanced period is meant to bridge the gap between two libraries, I'd say it's good to have those conversions over there.

I'll close the issue now, though thanks both for you input!