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
