Keith, Thanks for the review!!
John Keith Mitchell wrote: > Looks great now! > > Thanks, > Keith > > John Fischer wrote: >> Keith, >> >> Thank you for the review. I saw if part (line 912) this morning and >> said to myself that I need to fix it. So I have using your suggestion >> with the append. I also modified how the key width is calculated. The >> same part of code used to loop through the returned temporary dictionary >> to figure out the criteria key widths. Now I have the get_criteria_info >> return both the temporary list (tdict) and the criteria key width saving >> a loop. >> >> The slightly modified webrev >> >> http://cr.opensolaris.org/~johnfisc/list-output-13550/ >> >> Thanks, >> >> John >> >> >> Keith Mitchell wrote: >>> Lines 912, 916 and 917 shouldn't be needed now. sdict will always >>> have the name key. >>> >>> Sidenote: line 915 is redundant, and 913-914 could be reduced to: >>> >>> sdict[name].append(tdict) >>> >>> >>> - Keith >>> >>> John Fischer wrote: >>>> Jack and Keith, >>>> >>>> I have updated the webrev for this defect. >>>> >>>> http://cr.opensolaris.org/~johnfisc/list-output-13550/ >>>> >>>> to match the discussion. >>>> >>>> Thanks, >>>> >>>> John >>>> >>>> >>>> John Fischer wrote: >>>>> Keith, >>>>> >>>>> I have moved that line up and remove the if statement. I have >>>>> tested this on onol-inst.sfbay and it works for both the case >>>>> that revealed the bug and all other cases. >>>>> >>>>> Thanks, >>>>> >>>>> John >>>>> >>>>> >>>>> Keith Mitchell wrote: >>>>>> Why not start off the beginning of the for loop with "sdict[name] >>>>>> = []" >>>>>> >>>>>> Then the if/else at lines 911-916 isn't needed. >>>>>> >>>>>> - Keith >>>>>> >>>>>> John Fischer wrote: >>>>>>> All, >>>>>>> >>>>>>> Here is a simple code review for installadm list subcommand. >>>>>>> The current code uses the else clause for a for loop. The >>>>>>> name within the else clause is not defined within that context. >>>>>>> Removing the else causes the code to be in the main 'for name' >>>>>>> loop which has the 'name' defined within that context. >>>>>>> >>>>>>> http://cr.opensolaris.org/~johnfisc/list-output-13550/ >>>>>>> >>>>>>> This issue only shows up when a manifest has no criteria which >>>>>>> is why I missed it within the first code drop. >>>>>>> >>>>>>> Thanks, >>>>>>> >>>>>>> John >>>>>>> _______________________________________________ >>>>>>> caiman-discuss mailing list >>>>>>> caiman-discuss at opensolaris.org >>>>>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>>>> _______________________________________________ >>>>> caiman-discuss mailing list >>>>> caiman-discuss at opensolaris.org >>>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
