Thanks Keith! Just a couple of comments in-line...
Joe On 02/10/10 06:06 PM, Keith Mitchell wrote: > Hi Joe, > > Thank you for the re-review! My replies are inline. An updated webrev > will be sent out after addressing everyone's comments for the updated > webrev. > > - Keith > > On 02/10/10 01:52 PM, Joseph J VLcek wrote: >> >> >> I looked at the deltas to the screens files. I also compared the >> changes to the comments I had made in code review round 1. >> >> Most of this looks good to me. I only have a couple of small issues. >> >> Joe >> >> +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= >> >> cmd/text-install/osol_install/text_install/inner_window.py >> +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= >> >> >> Comment: >> -------- >> >> When invoking logging.log(5, ... >> >> It might be clearer to assign "5" to a meaningfully named variable >> >> e.g.: >> >> 440 logging.log(5, "InnerWindow.on_key_down\n" + str(type(self))) >> >> What does lvl 5 result in? > > That's a good point. This log level is defined as "INPUT" level logging > and assigned a variable in text_install.py, but it should be put > somewhere universally accessible, and then referenced. This logging > level was added to cover debug statements that would clog the log (due > to being in functions that are called frequently). (Ideally, this will > eventually get specified by the logging service, along with some of the > logging setup handled by text_install.py) > >> >> +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= >> >> cmd/text-install/osol_install/text_install/install_progress.py >> +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= >> >> >> Comment: >> -------- >> >> Could you please add a comment what the arguments represent? >> >> 55 self.status_loc = (4, 12, 50) >> 78 self.init_status_bar(6, 10, 50) > > I'll add a comment here (and briefly look into whether some/all these > values can be dynamically determined). These are simply setting the > location on screen where the progress bar will show up. > >> >> >> +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= >> >> DID cmd/text-install/osol_install/text_install/network_nic_configure.py >> +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= >> >> >> Comment: >> -------- >> >> 161 list_item.data_obj = label >> >> Still the same comment as I had from the 1st code review: >> >> Could the ListItem constructor set this even though it is in >> inner_window? > > Yes, I'm going to add it to the constructor. I know I previously > mentioned to you in private that I had another potential method for > changing this construct; I've decided to go with your suggestion. > >> >> Comment: >> -------- >> >> 230 # pylint: disable-msg=C0103 >> >> Is is going to be accepted practice to disable pylint messages >> or just allow a score less than 10> > > My personal belief is disabling is the better route - it's easier to > parse through the output of pylint if you know that all errors coming > out need to be addressed, rather than having to remember that for lines > x-y of file Z we can ignore message Q, and for line A of file B, the > message about C is acceptable. Good point! I agreed. > >> >> Also you didn't enable-msg=C0103 > > "pylint: disable-msg" should only take effect for the subsequent python > statement (if such a statement is a block statement such as a class > definition or 'if' clause, it affects the entire block). > > (If this isn't happening in practice, then we'll need to file a bug > against whoever's compiling pylint for OSol, since they'll want to > update...) > >> >> Sure ... this is at the end of the file but perhaps disable-msg=C0103, >> for completeness... >> >> >> >> +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= >> >> DID cmd/text-install/osol_install/text_install/network_type.py >> +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= >> >> >> Comment: >> -------- >> >> From the 1st round code review I had comment: >> >> 184 def validate(self): >> >> Can you add a comment that this function overrides BaseScreen.validate? >> This applies to all functions which override base class functions. >> >> I thought we talked about this and you were going to add the commnet. >> Did I remember this incorrectly? > > As I recall, you rescinded that comment after realizing the use of pydoc > and other tools can be used to find methods that override parent class > methods. Among the reasons for not explicitly adding these comments: it > becomes a maintenance issue; if the class order ever changes, or as > sub/super-classes are added, those sorts of comments are highly prone to > becoming outdated or forgotten. > > Let me know if *I'm* the one mis-remembering, as that's very possible. Ah... yes! Thanks for reminding me! > >> >> >> >> +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= >> >> DID cmd/text-install/osol_install/text_install/partition_edit_screen.py >> +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= >> >> >> Comment: >> -------- >> >> From the 1st round code review I had comment: >> >> 122 def _show(self): >> and >> 214 def validate(self): >> >> Can you add a comment that this function overrides BaseScreen's? >> >> This applies to all functions which override base class functions. >> >> I thought we talked about this and you were going to add the commnet. >> Did I remember this incorrectly? >> >> >> >> +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= >> >> cmd/text-install/osol_install/text_install/users.py >> +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= >> >> >> Question/Comment: >> ----------------- >> >> 278 '''Ensure the username is not "root" or "jack"''' >> >> Why is user jack not allowed? > > We suffer from the same restriction as the GUI, currently - "jack" user > is on the media (due to the use of the SUNWslim-utils package); this > prevents us from being able to add a 'jack' user without jumping through > hoops. > > See also bug 13750. OK, Thanks
