Hi Jean,
As usual, I tend to look 'slightly' beyond what was actually touched
(though I think I restrained myself almost entirely to PEP8 this time).
Feel free to tell me certain changes are not in scope and/or file bugs
for later fixing.
Thanks,
Keith
General:
General (non-pep8 note) If everything is going to be a staticmethod (as
in, for example, DefaultModule and ValidatorModule), this doesn't need
to be a class.
PEP8 calls for multiline docstrings to have a blank line before the
closing quotes, e.g.,
'''I'm a multi-line
docstring
'''
One line docstrings should be all on one line, including the closing
quotes, e.g.,
'''Sing line docstring'''
transfer_defs and dc_defs: defining __all__ would greatly simplify the
import statements in other modules.
Naming:
A lot of our functions and modules start with "DC_" (now "dc_"). Since
they are in the distro_const subdir of osol_install, prepending them
with dc_ is redundant. Since the names are all getting touched, perhaps
we could fix this convention?
DefaultsModule.py:
110: Change to:
if not uid_nodes:
return uid
77: Recommend:
return bool(non_root_users)
ValidatorModule.py:
62: Should be
return node.get_value().startswith("/")
Change 142 to:
return (value.lstrip("-.").isalnum())
and then remove the import string module and pylint disable/enable
194-198: Replace with:
return bool(node.get_value().strip())
distro_const/__init__.py:
1: Line isn't necessary (unless we directly exec/run this file somewhere
for some reason?)
dc_checkpoint.py:
285: Unnecessary. Change 290 to "append" instead of "insert"
306: "if not self.get_checkpointing_avail():"
309,310,316,323,539,711,746,769: Extra space before the colons
412: Split this into multiple lines for readability (and while you're at
it, use "splitlines()" instead of "rsplit("\n")", e.g.:
snapshot_list = Popen(cmd, shell=True, stdout=PIPE)
snapshot_list = snapshot_list.communicate()[0].splitlines()
473,504,506: Trailing slash is not needed
535: Change "if <all this> == False:" to "if not <all this>:"
536: Should be aligned with the open paren on 535 (it seems like there's
space)
583-584: Could be replaced with:
arglist.extend(cp.setp_list[currentstep].get_zfs_snapshot_list()
636: "if stop_on_err is None:"
642: "if not script:"
645,739: "if not cp.get_checkpointing_avail():"
744: "elif isinstance(var, basestring):"
754: "if os.access('/sbin/zfs', os.X_OK):"
dc_ti.py:
32,33: Standard library imports should be listed first.
262: Align this line with the open paren on 261
263: "if build_area is None:"
280: Align with the open quote on 279
dc_tm.py:
General: Names referencing auth(ority) could be changed to 'publisher'
as long as all of them are getting touched...
Multiple: Change "if not status == TM_E_SUCCESS:" to "if status !=
TM_E_SUCCESS:"
129: "if mirr_cmd is not None and pref_flag:"
136-141: Is there space to realign?
142: "if pref_flag:"
144: "elif mirr_cmd is not None:"
146: "elif refresh_flag:"
dc_utils.py:
57: Should be "if node_list and node_list[0]:"
85: "if node:"
113: "if str_val is None:"
distro_const.py:
75: This trailing slash seems out of place
124: Non PEP8: This whole function seems to be unnecessary
215: "if stepname is not None:"
217,253,265: "if step is None:"
260: "if pause:"
286,316,502: change to !=
292: "if resume and step_resume:"
299: "if do_list and (pause or resume or step_resume):"
307: "if do_list:"
417,561: "if dataset is not None:"
638-654: In Python 2.6, try/except/finally can be one block (with one
"try" instead of 2)
645: I think it would be appropriate to re-raise SystemExit here
(although given our usage of SystemExit in other code, perhaps not)
finalizer_checkpoint.py:
General: This looks like it needs an "if __name__ == '__main__':" clause
30: Should be after import shutil
66: Should be "raise Exception(..." instead of "raise Exception, (..."
finalizer_rollback.py:
General: Looks like it also needs an "if __name__ == '__main__':" clause
bootroot_archive:
135: "if manflist:"
282: "if BR_COMPR_LEVEL is None:"
288: "if BR_COMPR_TYPE is None:"
295: "if BR_PAD_SIZE_STR is not None:" (or, this could be removed and
the except below could catch TypeError, in which case line 292 is no
longer needed)
396: Bare "except" clause
grub_setup.py:
84: Unnecessary
92: "if RELEASE is not None:"
95-107: Could be converted to try/except/finally with Python2.6 (instead
of try/(try/except)/finally)
112-122: Ditto
121: "if RELEASE_FD is not None:"
124: "if RELEASE is None ..."
137: "if DEFAULT_ENTRY is None:"
143: "if TIMEOUT is None:"
205: "if position_str is None:"
jeanm wrote:
> Here's the code review for my Python2.4->2.6 and pylint changes for DC.
>
> http://cr.opensolaris.org/~jeanm/slim_2.6/
>
> Can I ask Karen to look at this, along with anyone else who wants to,
> since she's familiar with the DC.
>
> Jean
>
>
>
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
