Code review comment for lp:~nimit-svnit/gtg/dates

Hi!
    I have been having this problem since long. Last time Bertrand did my
work for me. But this time, I wish to do it myself.
      Whenever I do bzr merge ../trunk the revision number of my branch is
not updated. Can anyone tell me the probable cause?

    Another topic: Bertrand did you get a chance to read my last reply? Do
you agree with my points regarding the get_due_date method?

Nimit Shah,
B Tech 3rd year,
Computer Engineering Department,
SVNIT Surat
Secretary ACM-NIT Surat
www.dude-says.blogspot.com

On Thu, Jun 7, 2012 at 7:44 AM, Nimit Shah <email address hidden> wrote:

> Hi Bertrand!
> Thank you for the review!
>
> On Thu, Jun 7, 2012 at 4:03 AM, Bertrand Rousseau <
> <email address hidden>> wrote:
>
>> Review: Needs Fixing code inspection
>>
>> Some remarks:
>>
>> (line numbers are references from your patch)
>>
>> - I suggest you move line 36 in top of the method code, and remove the
>> line 22. It seems to me that the current code will update the due_date two
>> times if the start date comes after the new due date
>>
>
> You are right. I had made a small modification earlier. I should have
> moved the line up after I removed the modification.
>
>> - in line 27, you seem to test if the parent has a due date by doing "if
>> par.due_date", shouldn't it be "if par.due_date != Date.no_date()"?
>> - same remark for the children start date in line 34 ("if
>> sub.start_date")
>>
>
> Regarding the par.due_date != Date.no_date(), I just tried "if
> par.due_date" and it was working for me, hence I thought of keeping it that
> way. Anyways, I will modify it as that will be better to understand.
>
>
>> - you should use the accessor functions when you need to get the
>> start/due date from other object than the current (parents and children
>> tasks)
>> - if you unset the due date (fulldate == Date.no_date()), shouldn't you
>> skip all parent/children updates? Right now, you skip only the parent update
>>
>
> fulldate == Date.no_date check was required because of the comparisons of
> the dates. If I simply kept the comparison "par.due_date < Date(fulldate)"
> then when I would clear the due_date of child, the due_date of parent would
> also be cleared. Hence I needed to keep this check.
>
>
> - you shouldn't change the due date accessor, it seems to me that the
>> previous behavior was correct.
>>
>
> I'll explain the reason I removed the due_date accessor and was not using
> get_due_date.
> Earlier I was using the due_date accessor and the old implementation.
> But the problem was, get_due_date returns the most urgent due_date amongst
> it's parents but doesnot set the child's due_date equal to it. So, after
> setting the parent's due_date, when I ran the loop for child. The loop
> would run uselessly because, the get_due_date would return a due_date same
> as that of parent (in my case). Hence, even though the actual due_date of
> child would be later, the get_due_date accessor's behaviour made it
> impossible to update it without directly accessing the date.
>
> If we use the my modified get_due_date then the real due_date would be
> returned which will make it possible to view actually modify the child.
>
>
>> --
>> https://code.launchpad.net/~nimit-svnit/gtg/dates/+merge/108785<https://code.launchpad.net/%7Enimit-svnit/gtg/dates/+merge/108785>
>> You are the owner of lp:~nimit-svnit/gtg/dates.
>>
>
>
> Nimit Shah,
> B Tech 3rd year,
> Computer Engineering Department,
> SVNIT Surat
> Secretary ACM-NIT Surat
> www.dude-says.blogspot.com
>
>