Merge into old-trunk : dates : Code : GTG

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

Revision history for this message

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

#

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)

Revision history for this message

Nimit Shah (nimit-svnit) wrote on 2012-06-07:

#

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

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.

- 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

Revision history for this message

Nimit Shah (nimit-svnit) wrote on 2012-06-08:

#

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

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 <nimit.svnit@gmail.com> wrote:

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

Revision history for this message

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

#

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

On Fri, Jun 8, 2012 at 5:52 AM, Nimit Shah <nimit.svnit@gmail.com> 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 <nimit.svnit@gmail.com> wrote:
>
>> 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.
>>
>>
>>  - 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
>>
>>
>
> --
> https://code.launchpad.net/~nimit-svnit/gtg/dates/+merge/108785
> You are reviewing the proposed merge of lp:~nimit-svnit/gtg/dates into lp:gtg.

-- 
Bertrand Rousseau

Revision history for this message

Nimit Shah (nimit-svnit) wrote on 2012-06-08:

#

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

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 <
bertrand.rousseau@gmail.com> wrote:

> On Fri, Jun 8, 2012 at 5:52 AM, Nimit Shah <nimit.svnit@gmail.com> 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 <nimit.svnit@gmail.com>
> wrote:
> >
> >> 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.
> >>
> >>
> >>  - 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
> >>
> >>
> >
> > --
> > https://code.launchpad.net/~nimit-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.
>

Revision history for this message

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

#

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

On Fri, Jun 8, 2012 at 8:46 AM, Nimit Shah <nimit.svnit@gmail.com> 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 <
> bertrand.rousseau@gmail.com> wrote:
>
>> On Fri, Jun 8, 2012 at 5:52 AM, Nimit Shah <nimit.svnit@gmail.com> 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 <nimit.svnit@gmail.com>
>> wrote:
>> >
>> >> 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.
>> >>
>> >>
>> >>  - 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
>> >>
>> >>
>> >
>> > --
>> > https://code.launchpad.net/~nimit-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.
>>
>
> --
> https://code.launchpad.net/~nimit-svnit/gtg/dates/+merge/108785
> You are reviewing the proposed merge of lp:~nimit-svnit/gtg/dates into lp:gtg.

-- 
Bertrand Rousseau

Revision history for this message

Nimit Shah (nimit-svnit) wrote on 2012-06-08:

#

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

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 <
bertrand.rousseau@gmail.com> wrote:

> On Fri, Jun 8, 2012 at 8:46 AM, Nimit Shah <nimit.svnit@gmail.com> 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 <
> > bertrand.rousseau@gmail.com> wrote:
> >
> >> On Fri, Jun 8, 2012 at 5:52 AM, Nimit Shah <nimit.svnit@gmail.com>
> 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 <nimit.svnit@gmail.com>
> >> wrote:
> >> >
> >> >> 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.
> >> >>
> >> >>
> >> >>  - 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
> >> >>
> >> >>
> >> >
> >> > --
> >> > https://code.launchpad.net/~nimit-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.
> >>
> >
> > --
> > https://code.launchpad.net/~nimit-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.
>

Revision history for this message

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

#

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

> 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

review: Needs Fixing (code review)

Revision history for this message

Nimit Shah (nimit-svnit) wrote on 2012-06-08:

#

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

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 <
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
> You are the owner of lp:~nimit-svnit/gtg/dates.
>

Revision history for this message

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

#

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

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

Revision history for this message

Nimit Shah (nimit-svnit) wrote on 2012-06-08:

#

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

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 <
bertrand.rousseau@gmail.com> wrote:

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

Revision history for this message

Nimit Shah (nimit-svnit) wrote on 2012-06-09:

#

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

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 <nimit.svnit@gmail.com> 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 <
> bertrand.rousseau@gmail.com> wrote:
>
>> 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
>>
>> https://code.launchpad.net/~nimit-svnit/gtg/dates/+merge/108785
>> You are the owner of lp:~nimit-svnit/gtg/dates.
>>
>
>

Revision history for this message

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

#

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

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

Revision history for this message

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

#

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

review: Needs Fixing (code, run)

Revision history for this message

Nimit Shah (nimit-svnit) wrote on 2012-06-09:

#

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

Revision history for this message

Nimit Shah (nimit-svnit) wrote on 2012-06-09:

#

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

Revision history for this message

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

#

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

Revision history for this message

Nimit Shah (nimit-svnit) wrote on 2012-06-09:

#

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

Revision history for this message

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

#

Ok, approved

review: Approve

Revision history for this message

Nimit Shah (nimit-svnit) wrote on 2012-06-09:

#

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

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

GTG

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

Commit message

Description of the change

Preview Diff

Subscribers