On Fri, Jan 16, 2009 at 04:49:00PM -0800, Bart Smaalders wrote:
> http://cr.opensolaris.org/~barts/FatPkgs/
client.py:
- line 1847: I'd just as soon that -v be reserved for verbose. We don't
have that for image-create right now, but it's not impossible. I'd use
-V, I guess.
- line 1882: Get rid of the example; it can be in the man page. I'd
rewrite the remaining text, too, but I see you copied it from -a.
attribute.py:
- line 48: I thought you were going to get rid of the short form of this
action. I think it's actually necessary, now, as
set bob=fred
is ambiguous in some sense -- is it a set attribute that's been tagged
with the "bob" attribute, but missing the actual attribute data, or is
it just the short form of
set name=bob value=fred
generic.py:
- line 337: No space before "return", and capitalize.
- line 347: no need for parens
api_errors.py:
- line 109: You can't create objects as default arguments -- they get
created once and reused, which is not likely what you meant. You need
to set each to None, and then in the body of the method, test for None
and set to the empty list. It's also not clear what your comment on
111 means.
- line 148: This is bad i18n -- you can't just concatenate messages in
code like that. I also don't like the English. But nothing
significantly better is coming to mind.
badarch is treated unlike the other plan creation problems. The rest
allow multiple packages to fail plan creation, which is nice because
you don't have to modify your plan one package at a time to find out
all the bad packages. It's not clear to me that it can be any other
way, the way you raise the exception, but it might be good to consider
this when doing the SAT solver work, to see if there's a way to return
all the arch-broken packages you can in one shot.
image.py:
- line 293: I propose that variant attribute names follow the same
namespace the rest of the attributes do, except with "variant."
prepended. Thus, this should be "variant.opensolaris.zone".
- line 335: same problem here as in api_errors.py.
- line 1336: I think this needs a bit more documentation. Both in the
docstring (what does "excludes" mean? what does "represent" mean?
what does the "new_variants" signify?) and in the comment.
- line 1340: Don't use "vars" -- it's a builtin function.
imageplan.py:
- line 74: While you're here, remove the spaces around the equals signs.
pkgplan.py:
- line 118: same problem here as in api_errors.py.
- line 154, 155: commented-out?
variant.py:
- line 31: same problem as in api_errors.py. Though shouldn't this be a
full (self, *args, **kwargs), which you then pass on to the superclass
constructor? Wouldn't that obviate the need for the loop on 34,35?
- line 77-81: remove
manifest.py:
- line 121: Same problem here as in api_errors.py. Plus you might switch
the exclude lists so they match the order of the required arguments.
I wonder, though, given how much exclude lists are passed around, if
Manifest shouldn't just have an exclude list as a member, and that
unless you explicitly ask for an unfiltered manifest, you always get it
filtered (but the unfiltered one is what's stored in memory). Or do
you need to run different filters at different times on the same
manifest?
I also wonder how much our memory footprint will increase once we're
manipulating fat packages. The manifests should be, what, a little
less than twice the size?
- line 172: "manifest's"
- line 174: "all manifests"
- line 180: bad indent -- use line 190 as an example. Same for line
203ff.
- line 187, 188: delete
- line 194: space after comma.
- line 196: space after comment character
- line 209: no need for continuation character.
- line 217: Why not just create out as a tuple in the first place? You
should be able to do
out = tuple(chain(
(
[ m_dicts[i][k] for k in m_sets[i] - common_keys ]
for i in range(len(m_dicts))
),
(
[ m_dicts[0][k] for k common_keys ],
)
))
- line 219, 224, 242: same problem as in api_errors.py.
- line 247, 257: no need for semicolon
- line 281, 282: four space indent
- line 283: zero space indent
- line 285: ditch spaces around equals
- line 338: Hmm. Looks like we should define a semantic for multiple set
attributes with the same name.
- line 429: I think in other places we check if it's a list or not.
Danek
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
