Hi Sundar,
Thank you for the review again. Good questions too! Comment
in-line.
Thank you,
Clay
On Mon, 28 Sep 2009, Sundar Yamunachari wrote:
> *delete_service.py:*
> 542, 544: microroot --> boot archive
Fixed
> 749: Why we need the python2.4 here? I don't like the hard-coded value here.
As per bug 9804 (Need to ensure all python #! paths in install code have
version number) and further as Python keeps its modules on a per-language
version, this is strongly suggested:
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Python_Interpreter
Unfortunately, until this is in a contract I'd like to match as
specifically as possible. Hopefully these will be contract ran very soon
now and this will go away.
> *installadm_common.py:*
> 53, 65, 109: Suggestion: change the name of the booleans to make it clearer.
> comments ->skipComments, newlines->removeNewlines and
> blanklines->skipBlanklines
Good idea
> 288, 302: What is the difference between __getitem__() and gte()?
The method __getitem__() is a Python special function (as denoted by the
two underbars). It implements the list and dictionary style [] accessors.
While get() is a method one can call which unlike [] (which raise a
KeyError on a key not existing) will return None, or a default specified.
Regardless you seem to have honed in one why are they the same. I've fixed
them as they shouldn't be. They now follow the correct API:
def __getitem__(self, key):
'''
Provide a wrapped interface so one can run
if "/dev/dsk/c0t0d0s0" in dbfile_obj['DEVICE'] and get the most up to
date data for the file:
'''
# ensure we're getting accurate data
self._load_data()
# ensure key is valid
if not self.has_key(key):
raise KeyError(key)
# return a field object, populated from the dictionary
return self._result(self, key)
def get(self, key, default = None):
'''
Provide a wrapped interface so one can run
if "/dev/dsk/c0t0d0s0" in dbfile_obj.get('DEVICE') and get the most up to
date data for the file:
'''
# ensure we're getting accurate data
self._load_data()
# ensure key is valid
if self.has_key(key):
# return a field object, populated from the dictionary
return self._result(self, key)
# else return default
return default
> 641: enpty --> empty
Fixed
> 740-791: function clients(net)
> The output of pntadm -P <net> may have some entries missing some values. For
> example these four entries are from ins3525-svr. Check whether your code
> handles all the four entries.
>
> # pntadm -p 10.6.35.0
>
> Client ID Flags Client IP Server IP Lease Expiration Macro Comment
>
> 010003BA866375 03 10.6.35.180 10.6.35.8 Forever 010003BA866375 line2-280r
>
> 0100144F0057A8 03 10.6.35.103 10.6.35.8 Forever 0100144F0057A8 --> No
> comment field
>
> 00 05 10.6.35.247 10.6.35.8 Zero line2-x4100-sp --> No macro assigned
>
> 010007E923F7A6 05 10.6.35.179 10.6.35.8 06/28/2004 --> No macro or
> comment
Yup, this is handled as the tabs are still presented by pntadm for empty
fields. I set up an example on install.central and found:
>>> for key in a.DHCPData.clients("172.20.24.0").keys():
... print key
...
a.DHCPData.clients("172.20.24.0")[key][a.DHCPData.clients("172.20.24.0")["Client
IP"].index("172.20.27.23")]
...
Comment
'test'
Client IP
'172.20.27.23'
Server IP
'172.20.25.12'
Lease Expiration
'Zero'
Client ID
'00'
Flags
'00'
Macro
''
> *SUNWinstalladm-tools/prototype_com:*
> 46, 47: Why we need these definitions?
The symlinks are to allow usage messages to print out using the name of
the command called (which for now is delete-service/delete-client) while
the scripts themselves are delete_service.py and delete_client.py so that
Python can import them.
This will change when installadm(1) is converted to Python and all of this
goofiness can then go away.
> Thanks,
> Sundar
>
>
