On Fri, Apr 04, 2008 at 07:08:04PM -0700, Bart Smaalders wrote:
> http://cr.opensolaris.org/~barts/optional-dependencies/
In general, you seem to have lost whatever settings you were using to
expand tab indentations to spaces.
client.py
- line 895: extraneous tab
depend.py
- line 61: use a tuple
- line 84: no need for parens
- line 94: either put a space inside the brackets or don't. Need better
error message.
- line 105, 110: no need for continuation chars here.
- line 115: If you're being careful about KeyErrors here, then why not in
parse() and verify()?
- line 122: Perhaps
print >> sys.stderr, "Error generating search index: missing " \
"attribute '%s' in action '%s'" % (e[0], s)
directory.py:
- line 158: Keep the Windows comment here.
file.py:
- no need for this change
generic.py:
- line 152:
self.orderdict.update(dict((
(pkg.actions.types[t], i) for i, t in enumerate(ol)
)))
- line 430: will we ever return more than one?
legacy.py:
- line 50: should have a normpath() here, too, to match the one in
generic.py.
- line 64: get_name() may return a name with a slash in it, which likely
won't work when it's time to create the link. Should this be down
closer to where it's used?
- line 90: the reason you're doing all this is to reference count the
action, right -- so removal doesn't stomp on other packages which have
a legacy action for the same package? You might want to note that
here.
- line 142: use "if e.errno not in (errno.EEXIST, errno.ENOENT):"
image.py:
- line 125: maybe XXX before the last sentence?
- line 126: no space between comment and code.
- line 807: Why not just use the fmri constructor with the authority
keyword?
- line 823: duplicates work done on 819, though you don't do this in
apply_optional_dependencies().
- line 851: could you make the loop variable a three-tuple? That'll make
it more readable than using deps[X].
- line 1017: what kind of clever thing did you have in mind?
- line 1021: I'm wondering if perhaps we should have multiple lost+found
directories, each with a timestamp, rather than timestamping individual
files (fix the timestamp for the duration of the operation)? This
would make it easier to match pkg failures with places to look. Not
sure how this would interact with the gui.
- line 1026: to stderr, say "Warning:"
imageplan.py:
- line 162: set() should convert the generator on its own, without the
comprehension
- line 175: "var/sadm/install" is special for the legacy action, right?
- line 176: this could be part of the comprehension, too. And follow the
style I mention below for manifest.py.
- line 257: spaces around "="
- line 272: mismatched quotes
pkgplan.py:
- line 273: unnecessary change
fmri.py:
- line 183: you're not using dump_fmri() anywhere, and get_fmri() is
probably better anyway.
- line 211: I still think my hashing function is better, even if it's no
safer.
- line 293: all changes from this point onward aren't useful.
manifest.py:
- line 185: four-space indent for the contents of the comprehension, and
the close bracket should be in the same column as the start of the line
of the start of the expression.
publish.py:
- line 197, 198: line.strip()
- line 205: I'm confused here. What's the purpose of putting the
localpath attribute in here just to rip it out again, if the action
constructor doesn't do anything with it? If it's just junk, why not
use "NOHASH"?
- line 224: "if "path" in action.attrs:"
- line 228: do you really want a new Transaction for each file, rather
than one to cover the whole include?
Danek
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
