Keith, On 05/16/11 07:29 PM, Keith Mitchell wrote:
Despite the number of nits and various comments below, the code looks pretty good. The NS._show method (and its helpers) look great. Seems like there's now a backup for fixing UI related problems in the Text Installer!
Thank you for your vote of confidence. ;)
Hopefully it wasn't too much trouble working with that framework.
I found it to be elegant and quite useful.
Comments below.
Recommendations taken unless otherwise noted below. Updated webrev at http://cr.opensolaris.org/~wmsch/bug-7031613-1/ Differences at http://cr.opensolaris.org/~wmsch/bug-7031613-1diff
I like the notation, because it is distinctive and graphical, which helps recognition during development and maintenance. I don't think that the performance hit is significant in such an application, but I'll go along with your recommendation.- Keith General: When adding a single item to a list, you frequently use: mylist += [myitem] This constructs another list() object temporarily before concatenating it to mylist. Please use: mylist.append(myitem) to avoid creating the extraneous list.
I'd disagree that it is not intuitive. If the user requests 1 value, the user receives one value. If the user requests multiple values, the user receives a list.network_info.py:236-238: The function should be consistent with its return value - returning a list sometimes, and a string others, is non-intuitive.
Based on usage, I'd suggest leaving as is,
If left as is, _run_dhcpinfo() will return only one value.
and changing line 139 to carve out what's desired:
dns_server = self._run_dhcpinfo("DNSserv").splitlines()
dns_server = dns_server[:NSInfo.MAXDNSSERV] ns_info.py:We've got plenty of bits on disk and CD
not so many characters per line, though, but point taken.
- please use longer, more descriptive names for the file, contained classes, and class attributes, and follow PEP-8 naming conventions (separate words with underscores). Suggestions:... On 05/ 5/11 08:32 AM, William Schumann wrote:Requesting code review for adding Name Services to SCI Tool http://cr.opensolaris.org/~wmsch/bug-7031613/
_______________________________________________ caiman-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
