Hi Alok. I've snipped most verbage, leaving only items I had further comments on...
On 03/17/11 11:18 AM, Alok Aggarwal wrote:
OK... My point was that there are many function/method header docstrings missing. However, it is pylint, not pep8, which requires docstrings. If all that is needed is pep8, then OK...usr/src/lib/install_target/physical.py --------------------------------------Doesn't pep8 compliance require at least a ''' header? (what is proper term for this?)Either ''' or """ - I think we're mostly consistent in using """.
58: partition_sort may be misnamed, as it could be used by any object with a name attribute. Disk objects or anything that subclasses DataObject has a name attribute. (actually a property)Can you suggest a better name?
compare_by_name() ?
OK. I figured it would be simpler to raise an exception or fail after checking if overwrite was false and snap in self.snapshot_list, but actually what you have is less code. Furthermore, since errors here are programming errors, they won't happen often (if ever) anyway.usr/src/lib/install_target/logical.py ------------------------------------- I don't understand the logic of snapshot() on 428.If overwrite is true, the old snapshot is destroyed before a new snapshot of the same name is taken. But if overwrite is false, still a new snapshot of the same name is taken, which will fail. Why not just err out if an existing snapshot exists and overwrite is false? Or am I missing something?If overwrite is false and a new snapshot of the same name is taken, I believe that means there's a programming error and we want the command to fail in that case.
Thanks so much for the review, Jack. Alok
You're welcome!
Jack
Webrev location: http://cr.opensolaris.org/~aalok/cud_ti/
_______________________________________________ caiman-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
