Drew (& John),
Many thanks for a thorough review.
Response below. Mostly, I've accepted your suggestions/
corrections. In one case (remove .keys()) I couldn't and in
several cases, I've left it for John to decide.
On 06/03/11 17:39, 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?
OK - will move these lines to top of import list in all files where
they occur.
NIT: Please organize your imports per pep8 rules (pygtk import is
clearly not applicable here)
OK - so will now have up to 4 ordered lists of imports:
pygtk; standard; 3rd party; project
(Note: the __builtin__ command in __init__.py needs to be where it is.)
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.
Agreed - removed.
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.
OK - will do.
usr/src/cmd/gui-install/src/base_screen.py
------------------------------------------
60: commented line?
removed
145: You don't need .keys()
removed
usr/src/cmd/gui-install/src/confirm_screen.py
--------------------------------------------
99: make this line a simple 4 space indent
done
usr/src/cmd/gui-install/src/disk_screen.py
------------------------------------------
137-138: empty docstring
Updated to:
''' Show the Disk Screen.
This method is called when the user navigates to the screen
via the Back and Next buttons.
'''
193: does the UI prevent it? :)
Yes - I have changed the comment to:
UI will prevent this happening, but this check remains for completeness.
202: does TC prevent it? :)
Yes - comment amended, as per previous comment.
287, 295: unneeded parens
removed.
421: join to line 420
done
431, 432, 435: NIT: boolean args
fixed
617-619: 8 space indents
fixed - actually, this portion of code has moved due to another bug fix
647: if not slices:
done
831: join to line 830
done
844: join to line 843
done
usr/src/cmd/gui-install/src/failure_screen.py
---------------------------------------------
67-69: these should all fit on one line
done
usr/src/cmd/gui-install/src/fdisk_panel.py
------------------------------------------
118: use dict()
done (here, and throughout)
837, 844, 851: remove .keys()
No - this doesn't work - it leads to:
RuntimeError: dictionary changed size during iteration
(I guess GObject.handler_block() deletes the handler object?)
882: use list()
done (here and throughout)
912: s/priomary/primary
fixed
1196: s/ENTENDED/EXTENDED
fixed
usr/src/cmd/gui-install/src/finish_screen.py
--------------------------------------------
79-81: make this all one line
done
124: remove line
126: change cmd to [REBOOT]
done (also changed "cmd" 6 lines down)
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
removed
usr/src/cmd/gui-install/src/install_profile.py
----------------------------------------------
51, 52: use list()
fixed
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.
fixed
usr/src/cmd/gui-install/src/map.py
----------------------------------
132-134: maybe move this to a logging.debug call?
done
206: use list()
done
358, 397: commented line?
changed to debug call, as per 132-134
513, 560, 612: use list()
done
usr/src/cmd/gui-install/src/progress_screen.py
----------------------------------------------
120-122: join to one line
done
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
leaving for John.
195: use list()
done
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.
leaving for John
301-302: combine the calls
done
usr/src/cmd/gui-install/src/screen_manager.py
---------------------------------------------
106: use list()
done
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 :)
yes, please! When they're in Target, I'll remove from here.
Thanks
77: use dict()
done
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.
will leave until you add to Target classes.
166-167: combine the calls:
ctrl_type = ctrl_type.split()[0].upper()
done
usr/src/cmd/gui-install/src/timezone.py
---------------------------------------
307-309: commented lines?
removed
usr/src/cmd/gui-install/src/user_screen.py
------------------------------------------
97-99: join to one line
done
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....
leaving for John
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
leaving for John
275-289: Again, we have a function used only once. Can we remove the
function call part of this?
leaving for John
usr/src/cmd/gui-install/src/welcome_screen.py
---------------------------------------------
50-52: join to one line
done
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 "=" ?
that module has now been removed from our code.
Thanks,
- Dermot
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
