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
