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

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