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

Hi!
   your point about unnecessary work is correct. I'll make the required
change.
   I felt that the get_due_date was creating a confusion.But like you said,
we should discuss this seperately. For the time being I will restore the
old get_due_date method. Also, since directly accessing due_date would not
be a good way,for the time being I can create a new method
get_actual_due_date. Do you think that would be proper?

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

On Fri, Jun 8, 2012 at 3:35 PM, Bertrand Rousseau <
<email address hidden>> wrote:

> Review: Needs Fixing code review
>
> > 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
> --
> https://code.launchpad.net/~nimit-svnit/gtg/dates/+merge/108785
> You are the owner of lp:~nimit-svnit/gtg/dates.
>