Skip to content

Add DateInterval::add and DateInterval::sub methods #390

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

Closed
wants to merge 6 commits into from

Conversation

bukka
Copy link
Member

@bukka bukka commented Jul 21, 2013

Adding and substraction of two intervals

@laruence
Copy link
Member

could you also add some test scripts?

@kaplanlior
Copy link
Contributor

@bukka , thanks for the fast response (:

@weltling
Copy link
Contributor

Tested and it's good so far on linux/windows. Though a couple of questions

  • if I add two intervals of 9999 years, i see a year with 5 places. Isn't it out of range?
  • I currently don't see a way to substract intervals. Tried to set invert = 1 still results in an addition. Adding a sub method might be an option too.

I think that two things should be fixed as they'll flood back anyway as bugs. Also please add more tests for such edge cases.

@bukka
Copy link
Member Author

bukka commented Jul 25, 2013

I have just added a sub method for subtracting.

I will have a look into the invert and add more tests for edge cases (it would be great if you could suggest some).

Not sure if I understand why year with 5 places is out of range? It seems still better than warning...?

Thanks for the feedback ;)

@weltling
Copy link
Contributor

With the year - that was a question ;) Rereading the docs it seems to be fine for Durations, see http://en.wikipedia.org/wiki/Iso8601#Years

The invert option is is use for instance when substracting an interval for a date. DateTime::add() is about how it works.

For the other edge cases - look through the docs. Normally timelib should do its work, but things happen, you know.

@bukka
Copy link
Member Author

bukka commented Jul 28, 2013

Just added support for invert.

It could be actually useful to add support for operators overloading (as it is already in gmp) and then do something like:
$inverval = $interval1 + $interval2
It looks much nicer.

What do you think?

@weltling
Copy link
Contributor

Testing a bit looks like the current implementation doesn't work correctly. Please consider the following snippets

$i0 = new Dateinterval('P1Y4D');
$i1 = new DateInterval('P1Y3D');
$i0->invert = 1;
$i2 = $i0->add($i1); /* -1 day */
var_dump($i2->format('%y-%m-%d %h-%i-%s'));
var_dump($i2->invert);

$i2 = $i0->sub($i1); /* -(2 years and 7 days) */
var_dump($i2->format('%y-%m-%d %h-%i-%s'));
var_dump($i2->invert);

$i0->invert = 0;
$i2 = $i0->sub($i1);
var_dump($i2->format('%y-%m-%d %h-%i-%s'));
var_dump($i2->invert);

Current output:
string(11) "2-0-7 0-0-0"
int(0)
string(12) "2-0--7 0-0-0"
int(1)
string(11) "0-0-1 0-0-0"
int(0)

Expected output:
string(11) "0-0-1 0-0-0"
int(1)
string(11) "2-0-7 0-0-0"
int(1)
string(11) "0-0-1 0-0-0"
int(0)

$i0 = new Dateinterval('P1Y4DT3H');
$i1 = new DateInterval('P1Y4DT3H2M');
$i2 = $i0->sub($i1); /* -2 minutes */
var_dump($i2->format('%y-%m-%d %h-%i-%s'));
var_dump($i2->invert);

$i2 = $i1->sub($i0);
var_dump($i2->format('%y-%m-%d %h-%i-%s'));
var_dump($i2->invert);

Current output
string(13) "0-0-1 23-58-0"
int(1)
string(11) "0-0-0 0-2-0"
int(0)

Expected output
string(11) "0-0-0 0-2-0"
int(1)
string(11) "0-0-0 0-2-0"
int(0)

These tricky cases seems to be not covered, please correct me when i'm wrong. It seems not to respect the overflows in subordinate units, like if there where 61 second, it should go up to 1 minute and 1 second. Also i think the logic in the last commit is too complicated, please try to avoid that nested if statements, making it flat will result more feasible code. The thing is much more simple )

About the operator overloading - AFAIR there is none of that in any of the datetime classes, so it were better to stay in what this PRs title says. The operator overloading should be done in one move where it's applicable for datetime, IMHO.

Thanks for your effort.

@bukka
Copy link
Member Author

bukka commented Aug 1, 2013

Thanks for testing. I fixed all mentioned problems and added more tests. Also the code looks a bit better now.

@weltling
Copy link
Contributor

This one is merge ready i think, good job!

@@ -70,3 +70,37 @@ timelib_rel_time *timelib_diff(timelib_time *one, timelib_time *two)

return rt;
}

#define timelib_rel_invert_member(rt, member) do { rt->member *= -1; rt->invert = !rt->invert; } while(0)
Copy link
Member

Choose a reason for hiding this comment

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

Please, no macros. They are a pain to debug and it's really annoying if their names are lower case too!

@derickr
Copy link
Member

derickr commented Sep 2, 2013

s a concept, I think this is not really a good idea. You can add normal
intervals such as y/m/d/h/i/s, but how would you add things to "next
weekday" or "friday 3 weeks" etc to each other? If there is a add/sub,
then that really needs to work as well and as it's virtually impossible to do that right... I'd be against adding this.

@php-pulls
Copy link

Comment on behalf of krakjoe at php.net:

This PR is not in very good shape today, the objections raised by datelib maintainer do not seem to be addressed, indeed seem to point out problems that are intractable, I'm closing this PR.

Since the objection from derick is so strong, and since there seems to be theoretical problems with adding this kind of functionality, I'm not sure if it's worth opening a clean PR.

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.

6 participants