Hi Keith,
Thanks for the feedback :)
On 09/ 8/10 07:20 PM, Keith Mitchell wrote:
> Hi Darren,
>
> I just looked at the new path related functions.
>
> Couple minor notes:
>
> install_common/__init__.py: Unless there's a valid reason for importing
> every module under solaris_install in one go, there's no need to define
> __all__ here.
Removed - also removed logging in the cud_dc gate, since I agree, there
shouldn't be a need to bring in all modules here.
>
> data_object/__init__.py:
> 589: "if" block shouldn't be necessary, if the "for" block is re-worked
> slightly:
>
> parent = self
> while parent is not None:
> my_path.insert(1, quote(parent._name, ""))
> parent = parent._parent
Almost works ;)
There is a need to stop one below the root node, so as to not include the root
node's own name in the path - since the path is relative to it.
But it's certainly cleaner, and I've changed it to:
parent = self
while parent is not None and parent._parent is not None:
my_path.insert(1, quote(parent._name, ""))
parent = parent._parent
>
> 681: use "if 'attribute' in kwargs" rather than a try/except block here
Done.
>
> 696/712: Unnecessary, if the exception will be merely reraised, don't
> bother catching it in the first place.
Done.
>
> 745-6: Nit: Might be slightly more pythonic if you used
> "class_name.split('.')" and then parsed based on the length of the
> result (>1 = fully qualified).
Changed it to be more pythonic w.r.t. the use of lists and strings ;)
It now looks like:
mods = class_name.split(".")
if len(mods) > 1:
mod_name = ".".join(mods[:-1])
class_name_only = mods[-1:][0]
else:
# Assume it's relative to own module, for now.
mod_name = DataObjectBase.__module__
class_name_only = class_name
class_not_qualified = True
>
> 755-759: Would it be worth it to attempt to import the module, or no?
I'm not sure it's worth it - if it's not been imported already then it's very
unlikely to be found in the DOC.
But if you think it's still worth doing please let me know...
>
> 762: Use hasattr rather than getattr and a try/except block here.
Done.
>
> The function doesn't appear to descend further (i.e., referencing
> "a.b.c" would seem to break it).
I'm not sure what you mean here.
If I specify a class as a.b.c.d.E then the module name is a.b.c.d - and this
is what appears in the sys.modules as the key.
In what way do you think it should descend?
>
> 776: nit: If there's space on the line to do away with the "match"
> variable (and just use "if
> DataObjectBase.__NAME_RE.match(value_string)), that'd be preferred
> (perhaps changing the variable from "value_string" to something slightly
> shorter).
Do you mean to do:
if DataObjectBase.__NAME_RE.match(value_string):
instead of:
match = DataObjectBase.__NAME_RE.match(value_string)
if match:
I'm not sure that works, because in each of the clauses inside the if, I'm
referring to the match variable too, e.g.:
args["name"] = unquote(match.group(1))
Is there a way to refer to the return value that the if checked without using
a variable? (as in perl with the $_ special variable)?
Thanks,
Darren.
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
