Keith,
Thanks for the review. Comments are inline. In general, anything that
might effect functionality I'm not going to do now. The risk is too high
and it does pass our standard we have set.
Jean
Keith Mitchell wrote:
> 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.
I talked with Jack about this and here was his response:
"I did this to provide flexibility in the general case, in case for some
reason a default setter or validator needed to save state.
For DC's case, the methods don't store state. This is why there is no
init method.
pylint can be appeased by making these methods functions. Instances of
DefaultsModule and ValidationModule can still be created and the
validator and default setter functions can be called the same way they
are now. I wrote a quick-n-dirty program to verify this (below).
... And if state is needed in the general case in the future, it is
still accommodated."
>
> 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'''
Done.
>
> 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?
I'm going to say no to both of these. In the future we should consider
that but it's just to invasive.
>
> 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())
>
I'm not going to change functionality at this point in time.
> distro_const/__init__.py:
> 1: Line isn't necessary (unless we directly exec/run this file
> somewhere for some reason?)
OK. I'll remove this.
>
> dc_checkpoint.py:
> 285: Unnecessary. Change 290 to "append" instead of "insert"
>
> 306: "if not self.get_checkpointing_avail():"
Not doing.
>
> 309,310,316,323,539,711,746,769: Extra space before the colons
Done.
>
> 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()
Not going to do this.
>
> 473,504,506: Trailing slash is not needed
Done.
>
> 535: Change "if <all this> == False:" to "if not <all this>:"
Not doing.
> 536: Should be aligned with the open paren on 535 (it seems like
> there's space)
Done.
>
> 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):"
Not doing.
>
> dc_ti.py:
> 32,33: Standard library imports should be listed first.
Done.
> 262: Align this line with the open paren on 261
Done.
> 263: "if build_area is None:"
Not doing.
> 280: Align with the open quote on 279
Done.
>
> dc_tm.py:
> General: Names referencing auth(ority) could be changed to 'publisher'
> as long as all of them are getting touched...
Done.
> Multiple: Change "if not status == TM_E_SUCCESS:" to "if status !=
> TM_E_SUCCESS:"
>
> 129: "if mirr_cmd is not None and pref_flag:"
Not doing.
> 136-141: Is there space to realign?
Done.
> 142: "if pref_flag:"
> 144: "elif mirr_cmd is not None:"
> 146: "elif refresh_flag:"
Not doing.
>
> dc_utils.py:
> 57: Should be "if node_list and node_list[0]:"
> 85: "if node:"
> 113: "if str_val is None:"
Not doing.
>
> distro_const.py:
> 75: This trailing slash seems out of place
Done.
> 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)
Not doing.
> 645: I think it would be appropriate to re-raise SystemExit here
> (although given our usage of SystemExit in other code, perhaps not)
>
Not going to do now. Should think about it though.
> finalizer_checkpoint.py:
> General: This looks like it needs an "if __name__ == '__main__':" clause
Not doing.
> 30: Should be after import shutil
Done.
> 66: Should be "raise Exception(..." instead of "raise Exception, (..."
Done.
>
> finalizer_rollback.py:
> General: Looks like it also needs an "if __name__ == '__main__':" clause
Not doing.
>
> 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)
Not doing.
> 396: Bare "except" clause
Done.
>
> 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:"
Jean
>
> 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
