Hi Jan, Thank you for reviewing! My responses are below.
- Keith Jan Damborsky wrote: > Hi Keith, > > I took a look at UI components + text-install.py (so that I have better > picture how things work together). > Please see my comments below - just couple of nits. > > Thank you, > Jan > > > text-install.py > --------------- > 35 import subprocess > > Since only call() is used from 'subprocess' Python module, > could we just import this one and simplify to call to it - e.g. > > 35 import subprocess > -> > 35 from subprocess import call > > 240,251 subprocess.call > -> > 240,251 call > > Ditto for 'platform' module - only processor() is consumed: > > 33 import platform > -> > 33 from platform import processor I prefer the first form, as it makes it more obvious where the 'call()' and 'processor()' functions are from in this case. I'm not tied to either form though. > > 101 if logname is not None: > -> > 101 if logname: I'm checking to make sure that the caller passed in a parameter (i.e., that we're not using the default parameter). In this particular case, either would work identically, since logname "should" be a string object, but I want to be consistent with other functions where I follow the same format. > > 122,123 - comment seems incomplete It is missing something. Oops. I'll fix it. > > > line 196, 233 - since we already switched to 2.6, those > comments should be dealt with - e.g. addressed or changed > appropriately. Thanks. I thought I'd caught most of the comments I'd scattered, but looks like I missed these. > > > base_screen.c > ------------- > 58,59,60 - just a nit - please add space between 'Installer' word > and subsequent question mark May I ask why? Question marks should be directly after the word. > > 62 - installer -> Installer (to be consistent with rest of the messages) Changed. > > > main_window.py > -------------- > Please add more comments in MainWindow pop_up method in order > to make life easier for future maintainers. I can do that. > > > Keith Mitchell wrote: >> Hi Glenn and all other reviewers, >> >> I have posted an updated webrev at: >> http://cr.opensolaris.org/~kemitche/text_v2/ >> >> >> UI Components: >> cmd/text-install/osol_install/text_install/action.py >> cmd/text-install/osol_install/text_install/base_screen.py* >> cmd/text-install/osol_install/text_install/color_theme.py >> cmd/text-install/osol_install/text_install/edit_field.py >> cmd/text-install/osol_install/text_install/error_window.py >> cmd/text-install/osol_install/text_install/inner_window.py >> cmd/text-install/osol_install/text_install/list_item.py >> cmd/text-install/osol_install/text_install/main_window.py >> cmd/text-install/osol_install/text_install/scroll_window.py >> cmd/text-install/osol_install/text_install/window_area.py >> >> * This file is also relevant to the "Screens" group >> >
