Hi William,
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! Hopefully it wasn't too much trouble working with that framework.
Comments below.
- 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.
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, 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 - 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:
ns_info.py: nameservice_info.py
NSInfo: NameserviceInfo or Nameservice
nsvidx -> nameservice_index
ns_type -> name_service
dnsserv -> dns_server or dns_address
dnssearch -> dns_search
ldapprofname -> ldap_profile
ldapprofip -> ldap_ip or ldap_profile_ip
ldappbchoice -> ldap_proxy_bind
ldappbdn -> ldap_bind_domain or ldap_domain
ldappbpsw -> ldap_bind_password or ldap_password
nisauto -> nis_auto
nisip -> nis_ip
77-88: Building a series of length one lists and adding them together
with += is a bit odd. I'd suggest just making this one long list
instantiation, e.g.:
result = [foo,
bar,
baz,
...,
zzz]
101: Nit, should use comma instead of % for formatting done in logging
statements, e.g.
LOGGER().info("DNSserv=%s", dns_server)
Reasoning for that: If you have code that is run frequently (say a
function call that is called in a loop, lots and lots of times), string
formatting operations are expensive and could slow down that code
significantly. If the formatting is just being done for logging
purposes, it may be for nothing, too (for example, if the logging level
is set to WARN but the statement is logging.debug()). The syntax
logger.debug("my message: %s", my_arg) defers the string formatting to
the logging module - if the statement would get ignored, the formatting
is skipped, and much time is saved.
It's a minor difference, *most* of the time irrelevant to what we do,
but a good habit to be in.
120: Nit: use ilist.append(val). No need to construct a new, single item
list, just for the sake of +=.
Or use a list comprehension here:
ilist = [val for val in self.dnssearch if val]
151: What's special about the value 1?
165: Ditto
225: This dict should be statically defined.
265-276: Statically define this somewhere.
profile/__init__.py:
Naming: NS_LABEL is probably fine as "nsv", but the property itself
should be something more descriptive like "nameservice"
summary.py:
171-174, 176-179: Nit: Simple loops like this condense well to list
comprehensions, such as:
dnslist = [ln for ln in nsv.dnsserv if ln]
185, 192: The comments help a little bit, but numbers don't "read" well,
for what you're getting at here.
ns.py:
47: My quick searching shows this MAXWIDTH variable is unused; please
remove it.
53, 59-65, 83-95: As with ns_info, more descriptive names would help
make this code significantly more readable (and maintainable)
98-165: Since the screens shown seem mostly wildly different, perhaps
this would be better broken up into separate classes. There doesn't
appear to be much, if any, overlap. The overlap that *is* there would be
better served by having a common parent class.
Breaking things up in such a fashion probably simplifies a lot of the
various checking to figure out what kind of screen "self" currently is.
173, 185: The initial assignment in __init__ of self.nsv = NSInfo() made
this a bit difficult to follow (it didn't appear to be getting assigned
at first). I'd suggest setting self.nsv = None in __init__, and
modifying line 173 to be "sc_profile.nsv = NSInfo()", and then finally,
change 182-185 such that the 'else' is removed, and self.nsv =
sc_profile.nsv.
This also avoids the 9 subsequent screens each creating their own
NSInfo() objects that aren't ever used.
304 and many other locations:
In other languages (particularly Asian ones), some characters will take
up 2 spaces on screen. As a result, len(<string>) isn't a safe way to
figure out how much on screen space will be used by the string.
What this means is that you'll have to replace len(<string>) with
textwidth(<string>), where applicable. textwidth() is in terminalui.i18n.
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/
Due to the near completion of PSARC/2011/044 Move Name Service
Configuration to SMF (RFE 6959149)
SCI Tool can soon support prompting the user for configuration of name
services. Options are DNS, LDAP, NIS or none.
Most new code involves the UI.
Thank you,
William
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
