Hi Drew,
Thank you for the code review. Please see my responses inline.
On 04/26/11 21:21, Drew Fisher wrote:
Karen,
I looked at most of the files, excluding test files.
General:
Please run pep8 per gate README instructions. You have quite a few
pep8 issues in the code that need to be addressed. I will try to
refrain from listing specific pep8 issues but one or two might slip
past... :)
Will do. :-)
doc.insert_children can take a single object so there's no need
to make a list of one element. I see this in a large number of DOC
manipulation lines.
OK. I have gone through all the files, and fixed all occurrences of this.
You've got a few import * lines in the files. If possible, can you
import exactly what you need? It makes debugging far simplier in the
future if we know exactly where Slice comes from instead of having to
check all the import * modules.
Yep, fixed all that as well.
Also, make sure you run pylint to pick up any unused imports, unused
variables or unreachable code. Take pylint output with a HUGE grain
of salt though. It likes to complain about things it shouldn't.
Don't strive for a perfect score either. It's not worth doing,
usually.
OK, will do.
usr/src/cmd/distro_const/checkpoints/boot_archive_configure.py
--------------------------------------------------------------
272: to keep things consistant, use CPIOSpec.UNINSTALL (this was
recently added to install_transfer/info.py)
Fixed.
278-280: For super long lines, a 4 space indent works best:
root_tr_software_node = self.doc.persistent.get_descendants( \
name=TRANSFER_ROOT, class_type=Software, not_found_is_err=True)[0]
Will fix this, and any other places.
320-356: Quick question here for a future RFE the SMF folks are
working on. Is this the spot where the installers specify exactly
which files/dirs are transfered to the system? I ask because there
will be a need to explictly *NOT* copy /etc/svc/repository.db to the
system. It would be awesome if it could be specified here.
Well, this is ONE of the sections where we specify which files/dirs
are transferred to the system. The other places are lines 271-304
in the same file, and also all the changes I made to the "pkg_img_mod.py"
file.
For the /etc/svc/repository.db file in particular, it's a little
tricky. Since it
will exist in BOTH the root archive and the solarismisc.zlib.
So, if you really want to specify in the .transfer-manifest.xml to
not copy it over, you would have to specify the "uninstall" CPIO spec
in the transfer-misc checkpoint. So, right after line 298 in
the boot_archive_configure.py file.
For that particular file, I think it might be cleaner to get it
removed by specifying it in the "cleanup_list" in the
CleanupCPIOInstall checkpoint.
usr/src/cmd/distro_const/checkpoints/pkg_img_mod.py
---------------------------------------------------
276: see line 272 in boot_archive_configure.py
Fixed.
285: commented line?
Left over from debugging. Removed.
usr/src/cmd/text-install/solaris_install/__init__.py
----------------------------------------------------
97: Please use dict() instead of {}
Done.
55, 228: Why not use install_target/libbe/be.be_list() ?
I almost can. However, be.be_list doesn't return information
on whether the BE is active on next boot or not. I need
to find the BE that is active on next boot. So, until we
improve be.be_list() to return that information, I can not use it yet.
269-288: Cool. :)
usr/src/cmd/text-install/solaris_install/disk_selection.py
----------------------------------------------------------
224-225: 4 space indent (like boot_archive_configure.py:278)
Fixed.
234: get_children() returns a list even if there are no children.
if not self.disks:
fixed.
should be all you need.
426: same as 234
Fixed.
usr/src/cmd/text-install/solaris_install/disk_window.py
-------------------------------------------------------
115: s/option/optional
fixed
211: can simply change to: if part_list:
fixed
215: can simply change to: if slice_list
fixed
707: change [] to list()
fixed
802, 865, 885: change "gb" to Size.gb_units (to be consistent)
Fixed
862, 881: change "mb" to Size.mb_units
fixed.
usr/src/cmd/text-install/solaris_install/fdisk_partitions.py
------------------------------------------------------------
162: see disk_selection.py:234
fixed
209: This .. should fail. all_parts will always be a list (it might
be empty though) so, it should change to:
if not all_parts:
found_parts = False
else:
found_parts = True
Fixed as suggested.
usr/src/cmd/text-install/solaris_install/install_progress.py
------------------------------------------------------------
146, s/,/as
This code is actually removed now due to Keith's comment about
not using any threads in this file.
usr/src/cmd/text-install/solaris_install/log_viewer.py
------------------------------------------------------
73-82: Can you simplify this with a with() clause?
try:
with open(self.install_data.log_location, "w") as log_file:
log_data = logfile.read()
except (OSError, IOError) as error:
self.log_data = _(.......)
Yep. Fixed as suggested.
usr/src/cmd/text-install/solaris_install/partition_edit_screen.py
-----------------------------------------------------------------
191-193: 4 space indent for long lines
fixed.
usr/src/cmd/text-install/solaris_install/progress.py
----------------------------------------------------
37: change the argument list to default hostname="localhost" so you
don't have to do lines 38-41
Fixed.
71: no need for this line
Fixed
99: s/,/as
Fixed
105: use list()
Fixed.
137: extra line
Fixed.
usr/src/cmd/text-install/solaris_install/summary.py
---------------------------------------------------
182: change to dict()
Fixed
203: commented line
Fixed.
usr/src/cmd/text-install/solaris_install/ti_install.py
------------------------------------------------------
118: change to list()
This line is now deleted due to other changes.
236: The default action is "create" so you can remove this line
Fixed.
237: add_zvol has a 'use' argument that you can set to "swap" so this
line can be removed
Fixed as suggested
241, 242: same as 236, 237
Fixed as suggested
247: Use action=IPSSPEC.UNINSTALL instead. (This was just recently
put back)
Fixed.
37, 312: Why not use install_target/libbe/be.py ?
Fixed to use the be_unmount() from install_target/libbe/be.py.
usr/src/cmd/text-install/solaris_install/ti_install_utils.py
------------------------------------------------------------
309, 313: s/,/as
Fixed
323: s/kb/Size.kb_units
Fixed.
333-343: Whoa. A os.popen() call! :) Can we update this to use
solaris_install.Popen() instead?
memory_size = 0
p = Popen.check_call(["/usr/sbin/prtconf"], stdout=Popen.STORE,
stderr=Popen.STORE, logger=LOGGER)
for line in p.stdout.splitlines():
if "Memory size:" in line:
memory_size = int(line.split()[2])
break
and get rid of all this try/execpt and os.popen() stuff?
I will actually get rid of this function all together here since
all these swap/dump stuff is now moved to the target controller.
I will be sure to pass this info to Dermot so he can
get this fixed in the target controller.
usr/src/cmd/text-install/solaris_install/ti_target_utils.py
-----------------------------------------------------------
78-80: 4 space indent for long lines
Fixed.
83: get_children() returns a list even if there are no children.
86: if you want the first child, use get_first_child(). NOTE:
get_first_child does *NOT* return an empty list if the object has no
children. It returns None.
I changed to doing get_first_child(), I think it is cleaner.
91-93: 4 space indent for long lines
Fixed.
100-102: 4 space indent for long lines
fixed.
114-115: get_children() returns a list even if there are no children
Fixed.
133-134: get_children() returns a list even if there are no children
Fixed.
150-153: 4 space indent for long lines
fixed.
174-176: get_children() returns a list even if there are no children
fixed.
183-185: get_children() returns a list even if there are no children
fixed.
193-194: get_children() returns a list even if there are no children
fixed.
317: use list() instead of []
fixed.
362: get_children() returns a list even if there are no children
fixed
365: get_children() returns a list even if there are no children
fixed
514, 604, 618: use list() instead of []
fixed.
641: use Size.gb_units
fixed
699: get_children() returns a list even if there are no children
fixed.
825-843: Did you know install_target.physical.Partition has both of
these methods defined? I'm not sure if you can use them though, but I
wanted to make sure you knew they were available already on Partition
objects. is_solaris() is also defined there.
Yes, I can utilize those properties for the Partition object. However,
I still need to keep my UIPartition.is_logical() and
UIPartition.is_extended()
functions around because those are needed to take care
of "fake partitions".
932-933: you don't need all the parens
Fixed.
965: use list() instead of []
fixed.
976: outer parens aren't needed
fixed
1042-1043, 1046-1047: outer parens aren't needed
Fixed.
usr/src/lib/install_ict/__init__.py
-----------------------------------
70: bump to 57 per CR 7039499
Thanks. I will pick up that change when I resync next. This
change won't be included in final putback.
usr/src/lib/install_transfer/media_transfer.py
----------------------------------------------
Your C code is showing. :)
93: outer parens aren't needed
fixed
103-108: you don't need all the parens
fixed.
151, 154: outer parens aren't needed
fixed
179, 181: outer parens aren't needed
fixed
190-193: outer parens aren't needed
fixed.
301: change to: for line in p.stdout.splitlines()
fixed.
330: outer parens aren't needed
fixed.
361: commented line
fixed.
Thank you again for taking the time to review.
--Karen
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
