brick / date-time

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

Add `DefaultClock::travelBy(Duration)`, add `travelTo` to replace `travel` #92

Closed francislavoie closed 7 months ago

francislavoie commented 9 months ago

Closes https://github.com/brick/date-time/issues/91

francislavoie commented 9 months ago

One contentious thing I guess, I used ->abs() to make sure regardless of the Duration input, it goes semantically in the right direction.

Arguably we should maybe have travelDuration(Duration) instead? Then it's the user's responsibility to negate the duration if they want to go backwards for example.

francislavoie commented 9 months ago

I changed my mind, I think travelBy makes more sense. Short, and doesn't dictate whether it's positive or negative.

solodkiy commented 9 months ago

Personally, I like pair of travelForward and travelBackward more than play with negative Duration.

But I agree that ->abs in this case looks strange too, maybe Exception (assert) with comment that method accept only positive or zero Duration will be better.

BenMorel commented 9 months ago

But I agree that ->abs in this case looks strange too, maybe Exception (assert) with comment that method accept only positive or zero Duration will be better.

I'd definitely be against abs(). Either allowing a negative duration (that would change the direction of forward/backward), or throwing an exception, would be fine to me.

I changed my mind, I think travelBy makes more sense. Short, and doesn't dictate whether it's positive or negative.

I actually like it too. Maybe we could rename travel to travelTo() then?

francislavoie commented 9 months ago

Yeah I think travelTo makes sense, but isn't that a breaking change? Or do you mean we leave in the deprecated alias?

BenMorel commented 9 months ago

Yes, we would deprecate travel() and add travelTo() in the next patch version (0.6.1), and remove travel() in the next minor version (0.7.0)!

francislavoie commented 9 months ago

Okay - I'll adjust the PR to add travelTo, probably best if you set up the deprecation the way you want :+1:

francislavoie commented 7 months ago

@BenMorel this is ready to go on my end, anything else you need from me?

BenMorel commented 7 months ago

Thank you, @francislavoie!

BenMorel commented 7 months ago

Released as 0.6.4.