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

> 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.

fulldate == Date.no_date check is indeed needed, I did not say the opposite. What I meant is that if it happens to be true, then it seems to me that you can skip executing all the code that updates the parents AND the children. Right now you only make that test for the parents, not for the children, therefore performing unnecessary work (and it's possibly wrong too)

You should thus probably do something like this:

if fulldate != Date.no_date():
    # update parents
    (...)
    # update children
    (...)

>
> - 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.

I understand your issue. The simplest way here (but not the best way) is indeed to access the task due date directly, without the accessor. For simplicity, let's leave it at that now. The correct way would be to provide accessors for the actual due date value, and a different, 'smart', accessor which would provide the implicit due date, according to hierarchical constraints. But this require to modify all GTG code that is actually using the get_due_date method, which I'm not willing for you to do in this patch.

However, in your patch, there is another issue regarding get_due_date(): you seem to have modified this method (even though you're not using it), which is not ok. As some other parts of GTG rely on the previous behavior (probably the Work View and the column sorting code), I don't think you can change this without causing regressions. Indeed, as I already pointed out, since task.py is a central piece of code, changing get_due_date this require to consider the implications in the whole application. So please restore the previous get_due_date code.

>
> > --
> > 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