Merge lp:~nimit-svnit/gtg/dates into lp:~gtg/gtg/old-trunk

Proposed by Nimit Shah on 2012-06-05
Status: Merged
Merged at revision: 1196
Proposed branch: lp:~nimit-svnit/gtg/dates
Merge into: lp:~gtg/gtg/old-trunk
Diff against target: 46 lines (+17/-1)
2 files modified
CHANGELOG (+1/-0)
GTG/core/task.py (+16/-1)
To merge this branch: bzr merge lp:~nimit-svnit/gtg/dates
Reviewer Review Type Date Requested Status
Bertrand Rousseau (community) 2012-06-05 Approve on 2012-06-09
Review via email: mp+108785@code.launchpad.net

Description of the change

patch for bug #826916 date constraints

The date structure now works according to Bertrand awesome images and izidor's suggestion.
I have also modified get_due_date method as the previous version would cause confusion for the user when the proper date structure would be implemented

To post a comment you must log in.

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
 - 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")
 - 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
 - you shouldn't change the due date accessor, it seems to me that the previous behavior was correct.

review: Needs Fixing (code inspection)
Nimit Shah (nimit-svnit) 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

Nimit Shah (nimit-svnit) wrote :
Download full text (3.5 KiB)

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

Read more...

Download full text (4.1 KiB)

On Fri, Jun 8, 2012 at 5:52 AM, Nimit Shah <email address hidden> wrote:
> 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?

Yes, it's how bzr works: when you merge another branch in yours, it
imports the change from this branch, but it let you commit them (which
makes sens since sometimes you have to fix conflicts). Look at bzr
documentation to know more about its workflow.

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

I still have to some remarks, but I need more time.

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

Read more...

Nimit Shah (nimit-svnit) wrote :
Download full text (4.9 KiB)

   Yes that is correct. The problem is like this:
    trunk has revno = 1194 and my branch has revno=1190 and when I merge
and then commit the branch revno=1191 and not 1195. How to solve this
problem?

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 11:58 AM, Bertrand Rousseau <
<email address hidden>> wrote:

> On Fri, Jun 8, 2012 at 5:52 AM, Nimit Shah <email address hidden> wrote:
> > 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?
>
> Yes, it's how bzr works: when you merge another branch in yours, it
> imports the change from this branch, but it let you commit them (which
> makes sens since sometimes you have to fix conflicts). Look at bzr
> documentation to know more about its workflow.
>
> > 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?
>
> I still have to some remarks, but I need more time.
>
> > 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_da...

Read more...

Download full text (5.5 KiB)

On Fri, Jun 8, 2012 at 8:46 AM, Nimit Shah <email address hidden> wrote:
>   Yes that is correct. The problem is like this:
>    trunk has revno = 1194 and my branch has revno=1190 and when I merge
> and then commit the branch revno=1191 and not 1195. How to solve this
> problem?

It's normal, the two branches revision numbers don't have to be in
sync. When your branch will be merged in the trunk, the trunk revision
number will increment correctly.

> 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 11:58 AM, Bertrand Rousseau <
> <email address hidden>> wrote:
>
>> On Fri, Jun 8, 2012 at 5:52 AM, Nimit Shah <email address hidden> wrote:
>> > 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?
>>
>> Yes, it's how bzr works: when you merge another branch in yours, it
>> imports the change from this branch, but it let you commit them (which
>> makes sens since sometimes you have to fix conflicts). Look at bzr
>> documentation to know more about its workflow.
>>
>> >    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?
>>
>> I still have to some remarks, but I need more time.
>>
>> > 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())...

Read more...

Nimit Shah (nimit-svnit) wrote :
Download full text (6.2 KiB)

  oh okay!
   thank you!
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 12:55 PM, Bertrand Rousseau <
<email address hidden>> wrote:

> On Fri, Jun 8, 2012 at 8:46 AM, Nimit Shah <email address hidden> wrote:
> > Yes that is correct. The problem is like this:
> > trunk has revno = 1194 and my branch has revno=1190 and when I merge
> > and then commit the branch revno=1191 and not 1195. How to solve this
> > problem?
>
> It's normal, the two branches revision numbers don't have to be in
> sync. When your branch will be merged in the trunk, the trunk revision
> number will increment correctly.
>
> > 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 11:58 AM, Bertrand Rousseau <
> > <email address hidden>> wrote:
> >
> >> On Fri, Jun 8, 2012 at 5:52 AM, Nimit Shah <email address hidden>
> wrote:
> >> > 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?
> >>
> >> Yes, it's how bzr works: when you merge another branch in yours, it
> >> imports the change from this branch, but it let you commit them (which
> >> makes sens since sometimes you have to fix conflicts). Look at bzr
> >> documentation to know more about its workflow.
> >>
> >> > 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?
> >>
> >> I still have to some remarks, but I need more time.
> >>
> >> > 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 ju...

Read more...

Download full text (4.5 KiB)

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

Read more...

review: Needs Fixing (code review)
Nimit Shah (nimit-svnit) wrote :
Download full text (5.5 KiB)

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

Read more...

Download full text (6.3 KiB)

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

Read more...

Nimit Shah (nimit-svnit) wrote :
Download full text (7.0 KiB)

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

Read more...

lp:~nimit-svnit/gtg/dates updated on 2012-06-08
1191. By Nimit Shah on 2012-06-08

get_due_date restored and small modifications to set_due_date

1192. By Nimit Shah on 2012-06-08

latest code added

Nimit Shah (nimit-svnit) wrote :
Download full text (7.5 KiB)

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

Read more...

Ok, it seems to work right.

Some remarks still:
 - There seems to have been a conflict on CHANGELOG. You have to put line 9 under line 12, and remove lines 8,10,13.
 - on line 48 there is a tab instead of space

Playing with you branch, I identified what I think is another bug, but your patch is not related (I'm mentioning this here for the record). It's actually related to the get_due_date method, which we discussed earlier. Indeed, there's basically two different informations regarding the due date of a task: its actual due date, as defined in the task, and its constrained due date, as set by its parents hierarchy. Right now, the definition of get_due_date blurs the two. This has a side-effect: if you clear the due date, but the task parents have a due date, when calling get_due_date(), you will get the parent due date. Since this due date is sometimes also used to defined the task due date (notably when setting up the task editor), some manipulations in GTG will cause the task due date to be set although it should left undefined.

For instance, if you do the following:

 - undefine a child task due date (if previously defined)
 - define its parent due date to B
 - open the child task in the editor

What you expect is:

 - child task due date is undefined

What you get is:

 - child task due date is B

Consequences:

 - After that, the child task due date is set to B, where it should be left undefined

This is consistent with the interpretation of due dates in the previous GTG version, but I think this behavior is wrong in regard to our current discussion. Undefined due dates should stay undefined. The interpretation of undefined due dates is a matter of display logic policy and should not be enforced by explicitly defining the due date.

This can only be fixed by identifying the places in the code where we should access the actual due date and the constrained due date, and use a different due date accessor. This is a complex work. So here's what I suggest:

 - Fix the two items that I pointed out above
 - I'll merge your patch
 - I'll register a new bug for the "get_due_date bug" to provide a to discuss and manage that issue separately

review: Approve

Sorry, I wrongly chose "Approve", it's actually "Need fixing".. (you're very close, though, don't despair ;-) )

review: Needs Fixing (code, run)
Nimit Shah (nimit-svnit) wrote :

Hi!
   I'll make the required amendments.
   Your observation was the very reason I had modified 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 Sat, Jun 9, 2012 at 5:10 PM, Bertrand Rousseau <
<email address hidden>> wrote:

> Review: Needs Fixing code, run
>
> Sorry, I wrongly chose "Approve", it's actually "Need fixing".. (you're
> very close, though, don't despair ;-) )
> --
> https://code.launchpad.net/~nimit-svnit/gtg/dates/+merge/108785
> You are the owner of lp:~nimit-svnit/gtg/dates.
>

lp:~nimit-svnit/gtg/dates updated on 2012-06-09
1193. By Nimit Shah on 2012-06-09

Fix for bug #826916 by Nimit Shah

Nimit Shah (nimit-svnit) wrote :

  Bertrand: Is it okay now?
Nimit Shah,
B Tech 3rd year,
Computer Engineering Department,
SVNIT Surat
Secretary ACM-NIT Surat
www.dude-says.blogspot.com

On Sat, Jun 9, 2012 at 5:21 PM, Nimit Shah <email address hidden> wrote:

> Hi!
> I'll make the required amendments.
> Your observation was the very reason I had modified 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 Sat, Jun 9, 2012 at 5:10 PM, Bertrand Rousseau <
> <email address hidden>> wrote:
>
>> Review: Needs Fixing code, run
>>
>> Sorry, I wrongly chose "Approve", it's actually "Need fixing".. (you're
>> very close, though, don't despair ;-) )
>> --
>> 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.
>>
>
>

Almost: there is still a tab instead of a space in front of the first line of set_start_date ("self.start_date = Date(fulldate)")

review: Needs Fixing
lp:~nimit-svnit/gtg/dates updated on 2012-06-09
1194. By Nimit Shah on 2012-06-09

Bug fix for #826916 by Nimit Shah

1195. By Nimit Shah on 2012-06-09

Bug fix for #826916 by Nimit Shah

Nimit Shah (nimit-svnit) wrote :

hey!
   sorry I must have overlooked that line in the make lint output. I have
updated the code now. Have also removed those trailing whitespaces.

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

On Sat, Jun 9, 2012 at 6:01 PM, Nimit Shah <email address hidden> wrote:

> Bertrand: Is it okay now?
> Nimit Shah,
> B Tech 3rd year,
> Computer Engineering Department,
> SVNIT Surat
> Secretary ACM-NIT Surat
> www.dude-says.blogspot.com
>
>
>
> On Sat, Jun 9, 2012 at 5:21 PM, Nimit Shah <email address hidden> wrote:
>
> > Hi!
> > I'll make the required amendments.
> > Your observation was the very reason I had modified 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 Sat, Jun 9, 2012 at 5:10 PM, Bertrand Rousseau <
> > <email address hidden>> wrote:
> >
> >> Review: Needs Fixing code, run
> >>
> >> Sorry, I wrongly chose "Approve", it's actually "Need fixing".. (you're
> >> very close, though, don't despair ;-) )
> >> --
> >> 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
> You are the owner of lp:~nimit-svnit/gtg/dates.
>

Ok, approved

review: Approve
Nimit Shah (nimit-svnit) wrote :

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

On Sat, Jun 9, 2012 at 8:02 PM, Bertrand Rousseau <
<email address hidden>> wrote:

> Review: Approve
>
> Ok, approved
> --
> https://code.launchpad.net/~nimit-svnit/gtg/dates/+merge/108785
> You are the owner of lp:~nimit-svnit/gtg/dates.
>

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'CHANGELOG'
2--- CHANGELOG 2012-06-07 14:00:34 +0000
3+++ CHANGELOG 2012-06-09 13:51:20 +0000
4@@ -25,6 +25,7 @@
5 * Fix bugs #1003872 and #1002067: Correct titles to Preferences and Synchronization Services Dialogs, by Alan Gomes
6 * Improved Export plugin, export to PDF works now, by Izidor Matušov
7 * Added -t option to distinguish between multiple instances of GTG
8+ * Fix for bug #826916 by Nimit Shah
9
10 2012-02-13 Getting Things GNOME! 0.2.9
11 * Big refractorization of code, now using liblarch
12
13=== modified file 'GTG/core/task.py'
14--- GTG/core/task.py 2012-05-20 19:23:34 +0000
15+++ GTG/core/task.py 2012-06-09 13:51:20 +0000
16@@ -245,7 +245,20 @@
17 self.last_modified = modified
18
19 def set_due_date(self, fulldate):
20- self.due_date = Date(fulldate)
21+ self.due_date=Date(fulldate)
22+ if self.get_start_date() > Date(fulldate) and self.get_start_date() != Date.no_date():
23+ self.set_start_date(fulldate)
24+ if Date(fulldate)!= Date.no_date():
25+ for par_id in self.parents:
26+ par = self.req.get_task(par_id)
27+ if par.due_date != Date.no_date() and par.due_date < Date(fulldate):
28+ par.set_due_date(fulldate)
29+ for sub_id in self.children:
30+ sub=self.req.get_task(sub_id)
31+ if sub.due_date > Date(fulldate) and sub.due_date != Date.no_date():
32+ sub.set_due_date(fulldate)
33+ if sub.get_start_date() != Date.no_date() and sub.get_start_date() > Date(fulldate):
34+ sub.set_start_date(fulldate)
35 self.sync()
36
37 def get_due_date(self):
38@@ -262,6 +275,8 @@
39
40 def set_start_date(self, fulldate):
41 self.start_date = Date(fulldate)
42+ if Date(fulldate) > self.due_date and Date(fulldate) != Date.no_date():
43+ self.set_due_date(fulldate)
44 self.sync()
45
46 def get_start_date(self):

Subscribers

People subscribed via source and target branches

to status/vote changes: