The branch has been updated
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 4:30 PM, Nimit Shah <email address hidden> wrote:

> okay. Thanks!
>
> 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 4:25 PM, Bertrand Rousseau <
> <email address hidden>> wrote:
>
>> On Fri, Jun 8, 2012 at 12:43 PM, Nimit Shah <email address hidden>
>> wrote:
>>
>> > 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?
>> >
>>
>>
>> No, that name is not really clearer... until we found a better
>> organization, let's access the due_date directly.
>>
>>
>> >
>> > 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<
>> https://code.launchpad.net/%7Enimit-svnit/gtg/dates/+merge/108785>
>> > > You are the owner of lp:~nimit-svnit/gtg/dates.
>> > >
>> >
>> > --
>> > https://code.launchpad.net/~nimit-svnit/gtg/dates/+merge/108785<
>> https://code.launchpad.net/%7Enimit-svnit/gtg/dates/+merge/108785>
>> > You are reviewing the proposed merge of lp:~nimit-svnit/gtg/dates into
>> > lp:gtg.
>> >
>>
>>
>>
>> --
>> Bertrand Rousseau
>>
>> https://code.launchpad.net/~nimit-svnit/gtg/dates/+merge/108785
>> You are the owner of lp:~nimit-svnit/gtg/dates.
>>
>
>