Clay Baenziger wrote: > 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. okay. > >> *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: Yes. If they have to be same same why not let one call another. Now I understand the difference. > > 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 > '' As long as you take care of the empty value in one of the fields in the middle like macro, I am fine with it. > >> *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. okay.
Thanks, Sundar > >> Thanks, >> Sundar >> >>
