Hi Jan.
Jan Damborsky wrote:
> Hi Jack,
>
>
> Jack Schwartz wrote:
>> Hi Jan.
>>
>> Sure. Leave the bug ID in and push.
>>
>> Years ago when working on the USB team, I wanted to leave a bug ID in
>> a comment for the same reasons as Dave M, David C and you have
>> elaborated but was told not to. However, that was a different time
>> and I was working with a different set of people.
>
> Understood. Just out of curiosity, do you happen to remember
> if there was 'technical' justification for not doing this ?
> I am asking in order to realize if doing this might have some
> negative impact on the code.
There was no "technical" reason as far as I remember. The catalyst for
this, I believe is that at one point the code became too dirty with many
such bug references. (I remember sd in particular.) My guess is that
many references were intended to be there temporarily, but then some
bugs didn't get fixed and their reference were not removed.
Thanks,
Jack
>
> Thank you very much for your code review !
> Jan
>
>>
>> Thanks,
>> Jack
>>
>> On 04/29/09 01:36, Jan Damborsky wrote:
>>> Hi Jack,
>>>
>>>
>>> Jack Schwartz wrote:
>>>> Hi Jan.
>>>>
>>>> Thanks for making those changes.
>>>>
>>>> One other nit was introduced this iteration though...
>>>>
>>>> My understanding is that it is bad practice to put bug IDs into
>>>> code comments. This was done on line 885 of ict.py regarding the
>>>> dumpadm -r. Please remove the reference to 6835106 and file
>>>> another bug to document what needs to be fixed after 6835106 is fixed.
>>>
>>> I tend to agree with Dave in this point. In such cases, referencing
>>> bug number might save time and is only temporary - it is assumed
>>> that once related bug is fixed, workaround can be removed and comment
>>> modified appropriately. Thinking about this particular scenario, I had
>>> to go through the following process:
>>>
>>> Seeing in comments that I can't use 'dumpadm(1M) -r' and need
>>> to manipulate dumpadm.conf file directly instead, I had to
>>>
>>> [1] investigate why I can't use it
>>> [2] find out if the problem still exists
>>> [3] search if bug was already filed for this issue
>>>
>>> If bug had been already filed for this along with bug number
>>> mentioned in comments, I would have just taken a look at
>>> bug in order to determine if workaround was still needed or
>>> could be removed.
>>> In case was fixed, then workaround would be removed and comment
>>> changed appropriately.
>>> Based on this, I would rather leave reference to the bug number
>>> in comments.
>>>
>>> Please let me know what you think.
>>>
>>> Thank you,
>>> Jan
>>>
>>>>
>>>> No need to send me another review for this change.
>>>>
>>>> Thanks,
>>>> Jack
>>>>
>>>> Jan Damborsky wrote:
>>>>> Jack Schwartz wrote:
>>>>>> Hi Jan.
>>>>>>
>>>>>> I reviewed your code changes and they look reasonable to me.
>>>>>>
>>>>>> - I looked at the order that the new entries for
>>>>>> update_dumpadm_nodename() are called in install_finish, which
>>>>>> seems to be correct when I look at the existing call there for
>>>>>> the GUI install.
>>>>>>
>>>>>> - I'm OK with just pulling out the extra step of a temporary file
>>>>>> used to create dumpadm.conf (since this is a new system anyway
>>>>>> and dumps anyway would be critical to be able to use the system).
>>>>>>
>>>>>> My only nit is ict.py / lines 889- 890. Since you're copying
>>>>>> dumpadm.conf from a running system to another BASEDIR, I would
>>>>>> suggest something like this:
>>>>>>
>>>>>> dumpadmfile = '/etc/dumpadm.conf'
>>>>>> dumpadmfile_dest = self.BASEDIR + dumpadmfile
>>>>>>
>>>>>> ... but this is only a suggestion.
>>>>>
>>>>> Hi Jack,
>>>>>
>>>>> This is a good point. I have changed this according
>>>>> to your suggestion and updated the webrev.
>>>>>
>>>>> Thank you very much for reviewing those changes,
>>>>> Jan
>>>>>
>>>>
>>>>
>>>
>>
>
