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!
- 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. > > 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. > > +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= > > > 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. > > > 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. > > > > 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? > > 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. > > 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. > > 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. > > 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) > > 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. > > 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." > > 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? > > 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. > > 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? > > > 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. > > 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! > > > > >
