Keith Mitchell wrote: > Hi Joe, > > Thank you for taking the time to review. My comments are below. > Incidentally, you've brought up some very good points that have made me > go "hrmmmm," so I'd say it's a worthwhile review!
Thanks Keith. I'm glad my input was valuable. Thank you for your reply. I think all of your feedback to me is great. I have made some comments below. Joe > > - Keith > > Joseph J VLcek wrote: >> >> Hey Keith, >> >> Here is my feedback. Many of my comments are nit/questions. My Python >> experience is limited still. So many of my comments my be due to naivety. >> >> I may have learned more from doing this review than I provide. >> >> Hope this help. >> Joe >> >> --- >> >> +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= >> >> >> From code walkthrough: >> +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= >> >> >> >> Comment: >> -------- >> >> scroll_window.py >> >> I have a comment about init_scroll_bar. >> >> disk_window.py has: >> self.right_win.init_scroll_bar() >> >> Why init_scroll_bar is sometimes passed, FALSE, TRUE and >> nothing indicates to me this is not a BOOLIAN. It actually has >> 3 values. I think the argument should accept something like: >> >> DRAW_SCROLL_BAR, REMOVE_SCROLL_BAR, UNALTER_SCROLL_BAR > > While I see your point, I'm not sure I agree with the need to create a > separate enum-type setup to cover this scenario. Essentially, I've > combined functionality that could be separated into a single function > (i.e., "set/get_use_scroll_bar" and "repaint_scroll_bar"). If the > current setup makes usage non-obvious, I think splitting the > functionality would be simpler, and more readable. That is, have > "init_scroll_bar" always just read _use_scroll_bar, and then provide a > setter function for _use_scroll_bar that calls init_scroll_bar if > needed. Then, init_scroll_bar doesn't need the "3-way boolean," and the > setter would only take a "True" or "False" as an argument. Great! I like your solution. > >> >> Comment: >> -------- >> >> Why the x_loc/y_loc level window management? Isn't there already >> something which exists that can be used by Python to do this on >> a char. based screen? > > Interesting. Perhaps our initial research wasn't quite deep enough. I'll > look into it some, but I don't think rewriting at this stage will be > useful. > >> >> e.g.: http://excess.org/urwid/ > > Several of the more complex examples in their tutorial seem make use of > either relative or absolute x/y coordinates. Certainly, they do a better > job of abstracting some of it away then what was done here, but I can't > envision any UI framework that doesn't provide the option of explicitly > setting coordinates in some fashion. Thanks for considering this. I agreed that a UI framework likely needs this functionality but abstracting it as much as feasible seems valuable to me. That's the point I was trying to make. > >> >> +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= >> >> >> Comment from code I deskcheck reviewed: >> +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= >> >> >> >> I reviewed: >> >> cmd/text-install/osol_install/text_install/date_time.py >> cmd/text-install/osol_install/text_install/help_screen.py >> cmd/text-install/osol_install/text_install/network_nic_configure.py >> cmd/text-install/osol_install/text_install/network_nic_select.py >> cmd/text-install/osol_install/text_install/network_type.py >> cmd/text-install/osol_install/text_install/partition_edit_screen.py >> cmd/text-install/osol_install/text_install/timezone.py >> cmd/text-install/osol_install/text_install/timezone_locations.py >> cmd/text-install/osol_install/text_install/timezone_regions.py >> >> General comments >> ------------------------------------------------------------------------------- >> >> >> >> Comment: >> -------- >> Is this code supposted to be PEP8 ? > > Yes (see below). This code should be PEP8 and pylint clean, or nearly > so. I see scores mostly in the 9-10 range. > >> >> e.g.: >> pylint --include-ids=y --rcfile=../../tools/env/install.pylintrc >> help_screen.py >> ... >> Your code has been rated at 3.65/10 >> >> pylint --include-ids=y --rcfile=../../tools/env/install.pylintrc >> partition_edit_screen.py >> ... >> Your code has been rated at 0.75/10 >> >> pylint --include-ids=y --rcfile=../../tools/env/install.pylintrc >> network_nic_select.py >> ... >> Your code has been rated at -0.53/10 >> >> >> >> Comment: >> -------- >> Please add pydoc usable comments for each file. >> >> e.g.: >> % pydoc network_nic_configure.py >> no Python documentation found for 'network_nic_configure.py' > > For this, and pylint, you'll need to run from the > usr/src/cmd/text-install directory. Otherwise, both pydoc and pylint > can't resolve the imports properly. Pylint will work properly if you add > that directory to your PYTHONPATH, but pydoc will still get confused if > you reference the file directly. However, you can set the PYTHONPATH, > and then run "pydoc osol_install.text_install.network_nic_configure". I > imagine that other caiman code that imports osol_install.<something> > suffers similarly in a "clean" environment, but since I most of the code > is installed on the system already, it can at least reference the latest > release version. That brings up a good point, actually - one must be > sure to set the PYTHONPATH appropriately so that when we run pylint > before checkin, pylint is checking the updated versions of imported > modules, not the installed versions. Ah yes. Thanks! > >> >> >> Comment: >> -------- >> >> This is a comment I made during the code walkthrough sessions. I think >> the manipulation of the x & y coordinates should be done at a lower >> level. Pehaps a text_install/CoursePosition class could be defined. >> Maybe not worth it. But since it came up in the walkthrough sessions >> I thought I would note it. > > Do you see "CoursePosition" as a simple (x, y) "Point" type class, or > something more complex? Are you referring to the manipulation in the > screen code of, for example "y_loc += <something>"? I sort of see the > problem you're poking at, but I'm not sure what the solution might look > like yet. At least abstracting the x,y code... If evaluation results in more complete solution that might be valuable... > >> >> >> >> cmd/text-install/osol_install/text_install/date_time.py >> ------------------------------------------------------------------------------- >> >> >> >> Comment: >> -------- >> It seems you may be reinventing the wheel here. his seems like basic >> functionality >> to me. >> >> Perhaps exploring what Python base classes already exhist that could >> handle >> more of the functionality defined in date_time. > > The 'datetime' module is used in this file. Are there particular > instances in this file that you think deserve extra attention? I see where you use datetime and I took at look at what datetime provides and maybe I'm wrong but it seemed to me that you may be duplicating what datetime is doing/providing. > >> >> Comment: >> -------- >> It also seems to me that the x & y courser manipulation seels like basic >> functionality that might be already available somewhere. It's low level >> functionality that seems to me should belong at a lower level. >> >> Comment: >> -------- >> I found these varible names not very indicative: >> 55 YYYY = _("(YYYY)") >> YEAR maybe better > > How about YEAR_FORMAT? Given the number of other variables in this file > with "year" or "YEAR" in them, calling it just plain "YEAR" would be > more ambiguous. Sound good to me. > >> >> 60 OFFSET = 3 >> OFFSET to what? > > Answering the explicit question: Offset from the left side of the > window. Most/all screens have an OFFSET variable, and the UI elements > within the screens tend to be given OFFSET blank columns to their left. > > To answer the implied question: Removing the usage of OFFSET, and > instead doing something along the lines of adding a "border" (or > "left/right/top/bottom_border") property to InnerWindow, would condense > this and make it more intelligent. (border/offset/padding, something > along those lines). > >> >> Comment: >> -------- >> 61 YEAR_DIGITS = 4 >> >> Comment: >> -------- >> >> 84 self.year_edit = None >> 85 self.year_err = None >> 86 self.year_list = None >> >> _edit, _err, _list are repeated. Do you think it might be better to >> put these in a >> dictionary with keys: edit, err and list? > > There's a common paradigm in this and other code of "EditField inside of > a ListItem, with an associated ErrorWindow". I think we can take your > suggestion a step further and: > * Provide a common, easier way to construct an "EditField inside of a > ListItem with an ErrorWindow" globject. > * Strip these down to just "self.year_field" or something similar. The > year_list and year_err fields are rarely directly used, and if they're > needed, they could be referenced by, for example, > self.year_field.list_item and self.year_field.error_win, respectively. > >> >> Comment: >> -------- >> 178 self.edit_area.columns = 3 >> >> Hardcoding things like this raises a flag for maintenance issues. >> Would 60 OFFSET = 3 work here? > > OFFSET wouldn't be the right variable, but moving the hardcoded value to > a class property would definitely be preferred. > >> >> Comment: >> -------- >> 151 y_loc += 1 >> .... >> 260 self.center_win.add_text("(0-59)", y_loc, x_loc, >> self.win_size_x - 3) >> >> The code between lines 151 and 260 looks repetitious. Would it make sense >> to create parameterized supporting code which could be invoked >> multiple times >> >> to: >> >> As needed set things like: >> self.error_area.y_loc >> self.list_area.y_loc >> self.list_area.columns >> self.edit_area.columns >> self.edit_area.x_loc >> >> invoke: >> ErrorWindow() >> ListItem() >> EditField() > > Agreed. Part of this could be stripped down when the unified > "EditField/ListItem/ErrorWindow" concept from above is in place. > >> >> comment: >> -------- >> >> 326 def get_days_in_month(month_edit, year_edit, date_time): >> >> Could calendar.py monthrange be leveraged here? > > It is - see lines 347 and 350. The extra logic handles cases not covered > (for example, the user hasn't finished inputting the month or year yet, > so we need to have intelligent defaults given the remaining known data). > >> >> comment: >> -------- >> 345 if (len(cur_year) == 4 and year_valid(year_edit)): >> >> Could calendar.py isleap be leveraged here? > > This is checking to see if the user has input a valid year yet, e.g., > they could be halfway through typing "2010" and so there would only be a > "20" in the field. We don't actually want to look up the monthrange for > the year "20" in this case. However, it looks like the call to > year_valid is unnecessary and potentially toxic. Well at least my erroneous comment lead you to detect the potentially toxic part. ;) > >> >> cmd/text-install/osol_install/text_install/help_screen.py >> ------------------------------------------------------------------------------- >> >> >> >> Comment: >> -------- >> >> 67 self.help_info = [] >> 68 self.help_dict = None >> >> Does it makes sense to set these empty and to None when: >> 73 self.setup_help_data() >> will fill them in? > > Yes. Pylint and pydoc prefer variables to be 'announced' in the __init__ > function. > >> >> Comment: >> -------- >> >> 92 "NICSelect": ("network_manual.txt", >> ... >> 94 "NICConfigure": ("network_manual.txt", >> >> Are both of these supposed to have the same helpfile: >> "network_manual.txt"? >> >> Maybe yes eh? > > Yes. The one file contains the help text for both screens (the first > screen is not terribly complicated). > >> >> >> Comment: >> -------- >> >> 96 "TimeZoneRegions": ("timezone.txt", _("Time Zone")), >> 97 "TimeZoneLocations": ("timezone.txt", _("Time Zone")), >> 98 "TimeZone": ("timezone.txt", _("Time Zone")), >> >> Are all of these supposed to have the same header? I could see maybe the >> same help file but should the headers be different? >> >> Maybe no eh? > > Given that the text is the same, I think the headers should remain the > same. > >> >> Comment: >> -------- >> 105 if self.is_x86: >> 130 if self.is_x86: >> >> Could/Should these two conditionals be combined? > > Yeah they can. Should they? It's a readability issue, and the > performance difference would be unnoticeable if combined. Keeping the > help_info appends grouped and the help_dict entries grouped seems > preferable. OK. Thanks for considering it. > >> >> Comment: >> -------- >> 202 help_text = self.get_help_topic(info[0]) >> ... >> 207 help_topic = info[0] >> >> I see an inconsistency: >> >> help_text is set using get_help_topic yet help_topic is not. Is this >> wrong >> or is the named backwards? > > I'll have to defer to Sue on this one. > >> >> cmd/text-install/osol_install/text_install/network_nic_configure.py >> ------------------------------------------------------------------------------- >> >> >> >> Comment: >> -------- >> >> 101 find_defaults = self.nic.find_defaults >> >> Why is it necessarey to have this in a try/except block? You control >> self.nic.find_defaults. >> >> This may be a Python thing I just don't understand yet. > > No, it's wrong. It's a remnant of the original version of NetworkInfo, > for which this was the easier way to write things. The more appropriate > route would be to either add find_defaults to NetworkInfo.__init__ > (defaulting to True), or to change 100-103 to: > > find_defaults = getattr(self.nic, "find_defaults", True) Cool! I found one! ;) > >> >> Comment: >> -------- >> >> 157 self.center_win.activate_object(0) >> >> What does the "0" represent? Could a defintion be created for index 0 so >> an indicative name could be seen here? > > The 0 represents the first item in the list. We could do > "self.center_win.activate_object(self.ip_field)", which is *currently* > the first item in the list, but setting it to 0 here (and on other > screens) simply means "default to 0". Perhaps, though, > activate_object()'s first parameter should be optional, and default to > the "first item," instead. I like that approach. > >> >> Comment: >> -------- >> >> 168 list_item.data_obj = label >> >> Could the ListItem constructor set this even though it is in >> inner_window? > > InnerWindow.__init__ doesn't do anything with "data_obj" either, though > perhaps it should. Although in a more general sense, the data_obj field > is rather odd when I look at it again - it's used to generically link > some "data object" to the field for later manipulation. It's used mostly > for ListItems, to return the actual Python object associated with the > selected list item. It's also used in DiskWindow as a way to associate a > specific field with a specific partition (or slice) during editing. > > I'm open to suggestions on making this concept/need simpler, more > intuitive, and less "hacky." Maybe we could talk this through. I don't have the solution but if you think bouncing ideas off me could be helpful let me know. > >> >> Comment: >> -------- >> >> 170 validate = incremental_validate >> Is an arguement needed to incremental_validate? > > No. I want to pass in the function itself, not the result of the > function. EditField will call the function after each keystroke. > > >> >> Comment: >> -------- >> >> 238 def incremental_validate(edit_field): >> >> I find this name misleading. incremental_validate what? > > An IP address. So I suppose "incremental_validate_IP" would be better, yes? Yes, I think so. > >> >> Comment: >> -------- >> >> 222 def validate_ip(edit_field): >> 238 def incremental_validate(edit_field): >> >> Wouldn't these be better defined in ../profile/ip_address.py ? > > The "grunt work" is done in ip_address.py (IPAddress.convert_address and > IPAddress.incremental_check are called by these functions). The > functions here catch errors, and raise an error with a (potentially > translated) error message pertinent to the UI context in which the error > occurred. > >> >> Comment: >> -------- >> >> Support for IPv6? > > Not at this time. > >> >> cmd/text-install/osol_install/text_install/network_nic_select.py >> ------------------------------------------------------------------------------- >> >> >> >> Looks good. No new comments. >> >> cmd/text-install/osol_install/text_install/network_type.py >> ------------------------------------------------------------------------------- >> >> >> >> Comment: >> -------- >> >> 149 if self.install_profile.no_install_mode: >> >> >> I think it would be valuabel to add a comment describing what >> no_install_mode is - manual vs no_instal > > This check won't be in the final version, once the manual networking > pieces are available in the backend (it's in here in the meantime so > that QE can continue writing tests against the manual network config > screens). > > "no_install_mode" itself is explained in text-install.py and referenced > in install_progress.py. > >> >> 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'm not convinced that this is the best way to document such a thing. > From my perspective, I think methods from subclasses are generally > assumed to either override base class functions or provide extended > functionality beyond the base class. On the other hand, assumptions tend > to cause confusion. > > The pydoc for NICConfigure does a decent job of conglomerating the > necessary information - it shows the docstring for validate_loop() > (coming from BaseScreen), which calls itself out as the caller of > validate, and validate as coming from NICConfigure. > > I guess my main concern is that adding it to the comment of each > function that overrides another function seems like overkill, and seems > like there should be a more automated way to make those connections > obvious. Agreed. Now that I know how to run the pydoc. > >> >> cmd/text-install/osol_install/text_install/partition_edit_screen.py >> ------------------------------------------------------------------------------- >> >> >> >> Comment: >> -------- >> >> 81 self.slice_mode = slice_mode >> >> If this is this x86 specific would the name self.slice_mode_x86 be >> helpul? > > "self.x86_slice_mode", but yes, I agree adding the 'x86' portion makes > it more helpful. > >> >> Comment: >> -------- >> >> 85 if self.slice_mode: # x86, Slice within a partition >> ... >> 90 elif self.is_x86: # x86, Partition on disk >> ... >> 94 else: # SPARC (Slice on disk) >> >> Does this logic miss a possible error case where it's not slice_mode or >> is_x86 or SPARC? >> >> Suggestion: >> >> if self.slice_mode: # x86, Slice within a partition >> ... >> elif self.is_x86: # x86, Partition on disk >> ... >> elif self.is_sparc: <- would need to add this >> ... >> else: >> handle error/exception > > I don't quite follow. The only potential scenarios are: > x86 partitions > x86 slices > SPARC slices > > What other scenario were you thinking might occur? Ah OK in looking at this again I see. My mistake. > >> >> >> 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. >> >> Comment: >> -------- >> >> 139 logging.error('not skipping slice') >> >> Is this a: "all is well" message? If so it could end up confusing the >> user. > > This is an errant debugging message. It'll be removed. > >> >> >> Comment: >> -------- >> >> 143 except AttributeError: >> >> Whey doesn't this raise or report an error? Please add a comment >> describing >> this. > > In looking at this code again, I'm not sure why I don't re-raise and > fail here. I'll remove the try/except around this (or possibly re-raise > a more informative error message), as if this situation occurs, > something is broken. OK that's even better. > >> >> Comment: >> -------- >> >> 220 if self.is_x86 and not self.slice_mode: >> >> Is a condition to handle SPARC needed here? > > The "else" case covers slice editing for both x86 and SPARC in this > particular instance. > >> >> cmd/text-install/osol_install/text_install/timezone.py >> ------------------------------------------------------------------------------- >> >> >> >> Comment: >> -------- >> >> 132 if len(item[2]) > 0: >> ... >> 135 elif len(item[1]) > 0: >> >> Why not use this? >> >> if item[2]: >> ... >> elif item[1]: >> ... >> >> cmd/text-install/osol_install/text_install/timezone_locations.py >> ------------------------------------------------------------------------------- >> >> >> >> Comment: >> -------- >> >> 142 if len(item[2]) > 0: >> ... >> 145 elif len(item[1]) > 0: >> >> Why not use this? >> >> if item[2]: >> ... >> elif item[1]: >> ... >> >> >> cmd/text-install/osol_install/text_install/timezone_regions.py >> ------------------------------------------------------------------------------- >> >> >> >> 116 if len(item[2]) > 0: >> ... >> 119 elif len(item[1]) > 0: >> >> Why not use this? >> >> if item[2]: >> ... >> elif item[1]: >> ... > > Good idea on all 3 counts. > >> >> Comment: >> -------- >> >> No other new comments, except: >> >> The code in timezone.py, timezone_locations.py and timezone_regions.py >> seem to have a lot in common. Would developing a common base class, for >> shared code, make sense? > > Indeed it would. We'll get on it! OK Thanks! > >> >> >> >> >>
