GTG

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

dates
Merge into old-trunk

Revision history for this message

Bertrand Rousseau (bertrand-rousseau) wrote on 2012-06-08:

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

On Fri, Jun 8, 2012 at 12:43 PM, Nimit Shah <nimit.svnit@gmail.com> 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 <
> bertrand.rousseau@gmail.com> wrote:
>
> > Review: Needs Fixing code review
> >
> > > Hi Bertrand!
> > >    Thank you for the review!
> > >
> > > On Thu, Jun 7, 2012 at 4:03 AM, Bertrand Rousseau <
> > > bertrand.rousseau@gmail.com> 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

« Back to merge proposal