-
Notifications
You must be signed in to change notification settings - Fork 7.8k
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
Conversation
could you also add some test scripts? |
@bukka , thanks for the fast response (: |
Tested and it's good so far on linux/windows. Though a couple of questions
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. |
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 ;) |
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. |
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: What do you think? |
Testing a bit looks like the current implementation doesn't work correctly. Please consider the following snippets $i0 = new Dateinterval('P1Y4D'); $i2 = $i0->sub($i1); /* -(2 years and 7 days) */ $i0->invert = 0; Current output: Expected output: $i0 = new Dateinterval('P1Y4DT3H'); $i2 = $i1->sub($i0); Current output Expected output 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. |
Thanks for testing. I fixed all mentioned problems and added more tests. Also the code looks a bit better now. |
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) |
There was a problem hiding this comment.
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!
s a concept, I think this is not really a good idea. You can add normal |
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. |
Adding and substraction of two intervals