On Tue, Mar 17, 2009 at 11:45:17AM -0700, Bart Smaalders wrote:
> http://cr.opensolaris.org/~barts/CachedManifests/
image.py:
- line 795: m could be None here, so you're not benefiting from the loop
if you return immediately. For that matter, the intent setting is
false if m is None, too. Of course, if you're sure that get_manifest()
will always return useful data or raise one of the caught exceptions,
then it's okay; it just looks odd.
- line 896: No space after opening triple-quote. More of these in
manifest.py.
- line 914: Why are you treating arch specially from other variants?
- line 917, 945, 1798: Let's keep a blank line between the docstring and
the first line of a method's code.
- line 954: Could you change "encoutered" to "encountered" while you're
in the area?
- line 972: Might as well just open mode "w", and not bother with the
seek.
imageplan.py:
- line 29, 31, 32: No longer necessary, it appears.
manifest.py:
- line 36ff (ACTION_DIR, etc): Hey! I was watching that! ;-)
- line 454: You might want to explain that the directory cache isn't just
the directory actions, but also all the implicit directories implied by
other filesystem actions.
- line 460: It really isn't a pointer. set_image() is probably fine.
- line 477: use self.__image.get_manifest_path().
- line 483: the argument to KeyError should just be the key.
- line 488: This is a bit troublesome here -- we're marking the manifest
as having been loaded, but it's not actually usable yet, since its
contents haven't been filtered yet. Can that be an issue?
- line 492: blank line after this line
- line 494: you can use get_manifest_path() here, too. Scanning forward,
it seems you call this a lot -- why not cache the manifest path, and
possibly its dirname as well?
- line 498: would it ever be the case that manifest.dircache exists, but
the by-type manifests don't?
- line 505: __load() calls __finiload(), so this second call should be
extraneous.
- line 525: does self["publisher"] not work?
- line 546: I don't think you need to mention the ^C handling here.
- line 559: You should probably use mkstemp() here, just in case there
are battling processes.
- line 583: Wow. That (the argument to join() doesn't do what I'd expect
it to do. I don't know if it's obvious to anyone else, but the %
expando operator binds more tightly than the for.
- line 592: This already exists in the Image class. Make a common
utility function if you want, but don't copy the code.
- line 611: "vars"? I do not think this symbol means what you think it
means.
- line 615: Would it be any faster to put this code into the loop above?
- line 619: XXX, I think.
- line 635: Since we're going to the trouble to compute the contents of
manifest.dircache, shouldn't we go ahead and store it (if we can), too?
Same goes for gen_actions_by_type().
- line 663: Here, you should probably be making the test based on the
actual type you're looking for, rather than dircache.
- line 699: Should this be in the parent class, too? Same as in
__str__(), too, I guess.
- line 753, et al: return_line should be passed as a keyword argument
instead of a positional argument, just like it's passed in to this
method.
Danek
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
