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