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 DateComparableInterval #4063

Closed
wants to merge 1 commit into from
Closed

Conversation

nikic
Copy link
Member

@nikic nikic commented Apr 23, 2019

Following @derickr's suggestion in #4039, this changes DateTime::diff() to return a new DateComparableInterval class, which has well-defined comparison semantics.

TODO:

  • More comparison tests (only checked the tricky P1M case for now)
  • Naming? I've used DateComparableInterval here, Derick suggested DateAbsoluteInterval.

cc @frederikbosch @Antnee

@frederikbosch
Copy link
Contributor

Wonderful @nikic, I did not expect this PR/feature so soon!

Regarding the name, a class DateAbsoluteInterval suggests more features than DateComparableInterval. The latter, as the name indicates, suggests comparison only. So the question is whether we want to open this class for more than only comparison?

If we would name it DateAbsoluteInterval, which features would users expect from it?

  1. To be able to construct it. new DateAbsoluteInterval('PT1S')
  2. To be able to compare the objects.
  3. ....

So my suggestion would be to name it DateComparableInterval if the constructor is going to be private and usage is for comparison only, and name it DateAbsoluteInterval if the constructor is going to be public with the possibility of more features.

@nikic: why should we want to make the constructor private? I understand that the constructor should be limited if we want to make it public (probably (milli)seconds PT[0-9]*S only), otherwise we would lose the comparison feature.

@derickr
Copy link
Contributor

derickr commented Apr 23, 2019

I think we should work on an RFC for this first @nikic

@frederikbosch
Copy link
Contributor

@nikic @derickr Would you agree on my two alternatives? Then I will create a RFC that includes the two options to pick from.

@nikic
Copy link
Member Author

nikic commented Apr 24, 2019

@derickr First need to decide what the RFC is supposed to propose ;)

@frederikbosch A possible feature for DateAbsoluteInterval would be to get the total number of seconds.

why should we want to make the constructor private? I understand that the constructor should be limited if we want to make it public (probably (milli)seconds PT[0-9]*S only), otherwise we would lose the comparison feature.

We could allow anything up to (and including) days. I'm not sure how useful this would really be.

@nikic
Copy link
Member Author

nikic commented Jul 2, 2019

@derickr Any further thoughts on this?

@derickr
Copy link
Contributor

derickr commented Jul 9, 2019

@nikic Pretty much what it says in the ticket for the RFC?

@iluuu1994
Copy link
Member

@derickr Do you think this is still relevant? If not feel free to close this PR.

@derickr
Copy link
Contributor

derickr commented Jun 2, 2022

I don't know. This PR certainly no longer applies, and changing the return of DateTime::diff() is a BC break. I'm therefore closing this and if somebody is interested again we can revisit.

@derickr derickr closed this Jun 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants