On Mar 20, 2012, at 2:17 PM, Harold Shaw wrote: > Dave and Jesse, > Thanks again for the code review. Dave, Comments inline. > > I have updated the webrev and rerun the tests. > Webrev: https://cr.opensolaris.org/action/browse/caiman/hshaw/7147174_1 > > Harold
LGTM -Dave > > On 03/16/12 12:13, David Marker wrote: >> On Mar 9, 2012, at 4:13 PM, Harold Shaw wrote: >> >>> Happy Friday, >>> >>> Can I get a code review for the following bug: >>> >>> CR: http://monaco.sfbay/detail.jsf?cr=7147174 >>> >>> There is an additional change that isn't noted in the webrev. I moved line >>> 194 in target_selection.py left by one tab. The function, >>> __pretty_print_disk, if called with a list of disks would only create a >>> string with the first disk in the list. The return statement needed to be >>> moved out of the loop. >>> >>> Webrev: https://cr.opensolaris.org/action/browse/caiman/hshaw/7147174/ >> >> target_selection.py: >> >> Over all what you are doing looks right. But I would alter a few things. >> >> class DiskMatches: >> should inherit from object >> I would make nr_matches a property that just returns >> len(self.matches) > Good idea. Done >> 2988 can be removed if you make nr_matches a property. > Done. >> I believe 3020-3023 is covered later when no match is found at line >> 3046. So you don't need it here. > I waffled on this one a bit. The reason that it was there was to short > circuit the loop if no candidate disks were found. Since the list will never > be that long and because if does simplify the code I have removed the check. >> 3038 why not change it to append matched_disk.ctd ? >> This would also simplifty lines 3034-3035 as you don't need the >> list comprehension. >> Just `if matched_disk.ctd not in matched_disk_list:` would be >> enough. > That would be better. Done >> >> 3046: instead of checking here, you can take advantage of python's >> for/else: >> for candidate_disk in candidate_disk_list: >> … >> else: >> # you are guaranteed you didn't break out above so >> don't need to check >> # if candidate_disk.mapped_disk is None > Done. >> 3106 another extra check. you already raised a Selection error at 3048 > Done. >> >> test_target_selection_sparc.py: >> >> test_target_selection_props_1: >> >> OK so you are testing for the property FIXED. >> >> In order to make expected_xml a little more resilient (and >> since you don't care about size) I would replace the dev_size with a regex. >> That is line 1528: >> dev_size="\d+secs"/> >> >> Similarly on 1531 I would change it to: >> ......<size val="\d+" start_sector="\d+"/> >> >> If we were testing sizes I would leave them in, but this way if >> somebody changes c99t2d0 in DISCOVERED_TARGETS_XML you tests will still work. >> Also, mostly its ShadowLists job to test sizing things. >> >> test_target_selection_props_2_1: >> same, you are testing the dev_vendor not a size >> >> test_target_selection_props_2_2, >> test_target_selection_props_2_with_pools, >> test_target_selection_props_3_2, >> test_target_selection_props_3_3: >> you do specify a size for one of the disks, so you can leave >> its size in. but the other should also move to regex. >> >> >> test_target_selection_x86.py: >> >> in general same comments as the sparc version. rip out as many sector >> sizes as you can. > All done. >> >> cheers >> -Dave >> >> > _______________________________________________ caiman-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
