Dermot: I am going to you whether there is plan to remove usr/src/install/libict with GUI --> CUD. For ICT, are you going to use the ict that was converted to Python ???
I did not see this in your code review request. ----- Original Message ----- From: [email protected] To: [email protected] Cc: [email protected], [email protected] Sent: Friday, June 10, 2011 3:12:34 AM GMT -08:00 Tijuana / Baja California Subject: Re: [caiman-discuss] Code Review request: GUI Install -> CUD Thanks, Mary, I've fixed 29 pep8 nits in the source - virtually all of which were blank space-related issues. The results will be in the round 2 webrev coming out early next week. - Dermot On 06/10/11 00:32, Mary Ding wrote: > Dermot: > > In general, please make sure the *py files are pep8 compliance. There > is instructions in the slim_source usr/src/README to tell you how to > install and run pep8 to make sure they are clean. > > Thanks !!! > > > > On 06/ 3/11 09:39 AM, Drew Fisher wrote: >> Dermot, >> >> Here's my review of the entire webrev. For the most part this looks >> excellent! Great job! >> >> Anything I didn't comment on looks ok to me. I didn't look too hard >> at the XML files in $SRC/cmd/gui-install/xml, though >> >> -Drew >> >> >> >> >> >> >> NIT: For a lot of the Python files, you have this: >> >> 37 import pygtk >> 38 pygtk.require('2.0') >> >> Since this are hard-set requirements, shouldn't this import be done >> before anything else? >> >> NIT: Please organize your imports per pep8 rules (pygtk import is >> clearly not applicable here) >> >> NIT: Please nuke all of the pylint disable/enable comments. There's >> absolutely no reason at all to remain beholden to pylint's parsers and >> it just clutters the code. >> >> NIT - this is nothing more than a personal NIT on my part. When >> passing only boolean arguments can you label the arguments? >> >> e.g. >> self.set_back_next(back_sensitive=True, next_sensitive=False) >> >> This simply makes the code easier to read (for me. maybe not for you!) >> >> I see this a lot in the set_back_next() calls. >> >> >> usr/src/cmd/gui-install/src/base_screen.py >> ------------------------------------------ >> 60: commented line? >> >> 145: You don't need .keys() >> >> usr/src/cmd/gui-install/src/confirm_screen.py >> -------------------------------------------- >> 99: make this line a simple 4 space indent >> >> usr/src/cmd/gui-install/src/disk_screen.py >> ------------------------------------------ >> 137-138: empty docstring >> >> 193: does the UI prevent it? :) >> >> 202: does TC prevent it? :) >> >> 287, 295: unneeded parens >> >> 421: join to line 420 >> >> 431, 432, 435: NIT: boolean args >> >> 617-619: 8 space indents >> >> 647: if not slices: >> >> 831: join to line 830 >> >> 844: join to line 843 >> >> usr/src/cmd/gui-install/src/failure_screen.py >> --------------------------------------------- >> 67-69: these should all fit on one line >> >> usr/src/cmd/gui-install/src/fdisk_panel.py >> ------------------------------------------ >> 118: use dict() >> >> 837, 844, 851: remove .keys() >> >> 882: use list() >> >> 912: s/priomary/primary >> >> 1196: s/ENTENDED/EXTENDED >> >> usr/src/cmd/gui-install/src/finish_screen.py >> -------------------------------------------- >> 79-81: make this all one line >> >> 124: remove line >> 126: change cmd to [REBOOT] >> >> usr/src/cmd/gui-install/src/gui_install_common.py >> ------------------------------------------------- >> 162: Instead of opening the file with open(), use the linecache >> module: >> >> if os.path.exists(PIDFILE): >> previous_pid = int(linecache(PIDFILE, 1)) >> this_pid = os.getpid() >> <...> >> >> It'll save some indentation and will smartly handle errors (IOErrors, >> etc.) >> >> usr/src/cmd/gui-install/src/help_dialog.py >> ------------------------------------------ >> 43: commented line >> >> usr/src/cmd/gui-install/src/install_profile.py >> ---------------------------------------------- >> 51, 52: use list() >> >> usr/src/cmd/gui-install/src/libzoneinfo_ctypes.py >> ------------------------------------------------- >> Sweet! Another ctypes masochist! >> >> 39-48: Some of the types don't match what's in the header file. >> You've got c_int for some where you want c_uint. >> >> usr/src/cmd/gui-install/src/map.py >> ---------------------------------- >> >> 132-134: maybe move this to a logging.debug call? >> >> 206: use list() >> >> 358, 397: commented line? >> >> 513, 560, 612: use list() >> >> usr/src/cmd/gui-install/src/progress_screen.py >> ---------------------------------------------- >> 120-122: join to one line >> >> 156-174: I had no idea the TextFile class existed. Why the heck did >> Python stick that class in the distutils module instead of somewhere >> more universal?! >> >> I think it reads easier if you use the readlines() call though instead >> of a readline() while loop: >> >> urlimage_dict = dict() >> image_fh = TextFile(....) >> for line in image_fh.readlines(): >> if "=" in line: >> filename, sep, url = line.partition("=") >> filename = os.path.join(path, filename) >> urlimage[filename] = url >> return urlimage_dict >> >> 195: use list() >> >> 263-282: There's an entire in-line function setup to create a >> software node >> for CPIO that's used only once and only inside this class method. >> That tells me there's no reason for an in-line function here. >> >> 301-302: combine the calls >> >> usr/src/cmd/gui-install/src/screen_manager.py >> --------------------------------------------- >> 106: use list() >> >> usr/src/cmd/gui-install/src/target_utils.py >> ------------------------------------------- >> 29-34: Would it help if these were part of targets? I can do that, >> you know :) >> >> 77: use dict() >> >> 81-86: partition names should be strings and not ints. Not sure if >> this would cause a problem if they were strings in this section of the >> code though. >> >> 166-167: combine the calls: >> ctrl_type = ctrl_type.split()[0].upper() >> >> usr/src/cmd/gui-install/src/timezone.py >> --------------------------------------- >> 307-309: commented lines? >> >> usr/src/cmd/gui-install/src/user_screen.py >> ------------------------------------------ >> 97-99: join to one line >> >> 159, 171-172: You default data=None but then try to split it out. Is >> it possible to traceback here because of that? If this wasn't a pygtk >> callback method, I would think you'd have to add special handling for >> it but I'm not sure if pygtk takes care of it for you or not.... >> >> 283-285: We could use str.translate here to make this a lot more >> readable: >> >> import string # depricated, but that's ok >> # construct a map of viable characters for a hostname >> viable = string.letters + string.digits + "-_." >> if " " in hostname or hostname.translate(None, viable): >> return False >> return True >> >> 275-289: Again, we have a function used only once. Can we remove the >> function call part of this? >> >> usr/src/cmd/gui-install/src/welcome_screen.py >> --------------------------------------------- >> 50-52: join to one line >> >> usr/src/cmd/system-config/profile/user_info.py >> ---------------------------------------------- >> 59-62: I know this isn't yours, but can you remove the extra spaces >> from in-between the "=" ? >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> On 6/1/11 11:12 AM, Dermot McCluskey wrote: >>> Hi, >>> >>> I'd like to request a code review for the GUI Install -> CUD project. >>> >>> I need to set a timeout of Friday June 10th for this first round of >>> comments. >>> Please let me know if you intend to review, and also which of the >>> sections >>> listed below you will be looking at. >>> >>> The webrev is here: >>> http://cr.opensolaris.org/~dermot/webrev-cud-gui-round-1/ >>> >>> The latest iso and usb images for the project are here: >>> file:///net/indiana-build.us.oracle.com/var/lib/hudson/jobs/dc/cud_gui/media >>> >>> >>> >>> Below is my suggested grouping of the sources. If reviewers think a >>> code >>> walk-through would be helpful, then let me know. I could do this >>> at, eg, >>> 9am PT on Friday, 3rd or Monday, 6th or some other time if that doesn't >>> suit. >>> >>> >>> Thanks, >>> - Dermot >>> >>> >>> 1. Makefiles and manifest >>> ================== >>> usr/src/Makefile.master >>> usr/src/cmd/Makefile >>> usr/src/cmd/Makefile.targ >>> usr/src/cmd/gui-aux/Makefile >>> usr/src/cmd/gui-install/Makefile >>> usr/src/cmd/gui-install/src/Makefile >>> usr/src/cmd/gui-install/pixmaps/Makefile >>> usr/src/cmd/gui-install/xml/Makefile >>> usr/src/pkg/manifests/system-install-gui-install.mf >>> >>> 2. Glade UI Definition file >>> ================== >>> (These files are basically the old *.glade files which define the >>> Gtk+ widgets >>> and layout of each screen. They have been run through the >>> gtk-builder-convert >>> utility to convert them to the newer Gtk+Builder format, plus some >>> minor >>> tidying up.) >>> usr/src/cmd/gui-install/xml/confirmation.xml >>> usr/src/cmd/gui-install/xml/date-time-zone.xml >>> usr/src/cmd/gui-install/xml/failure.xml >>> usr/src/cmd/gui-install/xml/gui-install.xml >>> usr/src/cmd/gui-install/xml/installation.xml >>> usr/src/cmd/gui-install/xml/installationdisk.xml >>> usr/src/cmd/gui-install/xml/users.xml >>> >>> 3. App startup and checkpoint setup; handle screens; misc. >>> =========================================== >>> usr/src/cmd/gui-install/src/gui-install.py >>> usr/src/cmd/gui-install/src/__init__.py >>> usr/src/cmd/gui-install/src/base_screen.py >>> usr/src/cmd/gui-install/src/screen_manager.py >>> usr/src/cmd/gui-install/src/gui_install_common.py >>> usr/src/cmd/gui-install/src/help_dialog.py >>> usr/src/cmd/gui-install/src/install_profile.py >>> >>> 4. Welcome Screen and Confirmation Screen >>> ============= >>> usr/src/cmd/gui-install/src/welcome_screen.py >>> usr/src/cmd/gui-install/src/confirm_screen.py >>> >>> 5. Disk Screen >>> ========== >>> usr/src/cmd/gui-install/src/disk_screen.py >>> usr/src/cmd/gui-install/src/fdisk_panel.py >>> usr/src/cmd/gui-install/src/target_utils.py >>> usr/src/lib/install_target/controller.py >>> >>> 6. Timezone screen >>> ============= >>> usr/src/cmd/gui-install/src/timezone_screen.py >>> usr/src/cmd/gui-install/src/timezone.py >>> usr/src/cmd/gui-install/src/map.py >>> usr/src/cmd/gui-install/src/libzoneinfo_ctypes.py >>> >>> 7. User Screen >>> ========== >>> usr/src/cmd/gui-install/src/user_screen.py >>> usr/src/cmd/gui-install/src/user_support.py >>> >>> 8. Progress Screen / Perform Installation >>> =========================== >>> usr/src/cmd/gui-install/src/progress_screen.py >>> usr/src/cmd/gui-install/src/failure_screen.py >>> usr/src/cmd/gui-install/src/finish_screen.py >>> usr/src/cmd/gui-install/src/textview_dialog.py >>> usr/src/cmd/gui-aux/imageurl.txt >>> usr/src/cmd/system-config/profile/user_info.py >>> >>> _______________________________________________ >>> caiman-discuss mailing list >>> [email protected] >>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >> _______________________________________________ >> caiman-discuss mailing list >> [email protected] >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss > _______________________________________________ caiman-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
