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

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

Merged
merged 9 commits into from
Apr 24, 2024

Conversation

francislavoie
Copy link
Contributor

Closes #91

@francislavoie
Copy link
Contributor Author

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 francislavoie changed the title Add travelForward & travelBackward Add travelBy Feb 3, 2024
@francislavoie
Copy link
Contributor Author

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

@francislavoie francislavoie changed the title Add travelBy Add DefaultClock::travelBy(Duration) Feb 3, 2024
@solodkiy
Copy link
Contributor

solodkiy commented Feb 4, 2024

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

BenMorel commented Feb 4, 2024

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

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

@BenMorel
Copy link
Member

BenMorel commented Feb 6, 2024

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

francislavoie commented Feb 6, 2024

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

@francislavoie francislavoie changed the title Add DefaultClock::travelBy(Duration) Add DefaultClock::travelBy(Duration), add travelTo to replace travel Feb 6, 2024
@francislavoie
Copy link
Contributor Author

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

Copy link
Member

@BenMorel BenMorel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from a couple nitpicks, LGTM @francislavoie!

.gitignore Outdated Show resolved Hide resolved
tests/DefaultClockTest.php Outdated Show resolved Hide resolved
@BenMorel BenMorel merged commit 056d46c into brick:master Apr 24, 2024
7 checks passed
@BenMorel
Copy link
Member

Thank you, @francislavoie!

@francislavoie francislavoie deleted the travel-forward branch April 24, 2024 21:57
@BenMorel
Copy link
Member

Released as 0.6.4.

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

Successfully merging this pull request may close these issues.

Add DefaultClock::travelForwards(Duration)
3 participants