On Mon, Feb 09, 2009 at 03:33:29AM -0600, Shawn Walker wrote:
> http://cr.opensolaris.org/~swalker/pkg-2691/
Any chance you can cook up an auxiliary webrev that does diffs between the
old repository.py and the new depot.py? It's pretty hard to review these
changes as-is. I haven't reviewed it yet; if you can't or don't want to do
this, say so, and I'll figure it out myself.
depot.py:
- line 432: why not just "auto_create=not readonly"? Do you envision
other times when you won't want to auto_create?
action/__init__.py:
- line 106: why the change from position to position+1?
- line 113: I'd probably call this ActionDataError or something; having
"FileAction" in there makes it seem like only file actions would throw
this.
- line 158: perhaps "... and action.attrs.get(ka, None) is None"? I'd
speed test both, to see.
- line 160: drop the commas from the message
action/file.py:
- line 113ff: we do this a couple times now; is it worth breaking out
into a utility function? I might also bump the buffer size up to 128k.
action/generic.py:
- line 36: you might use cStringIO instead; it should be faster, and
maybe more memory efficient. Though I see at least in one place, you
changed it from cStringIO, so perhaps you have a reason?
- line 122: why is this a problem?
- line 155, 167: these clauses can be pretty bad on the memory usage.
Where do we end up using them? It might help to do something like
data = StringIO.StringIO(data.read())
data.seek(0, 2)
sz = data.tell()
data.seek(0)
- line 160: you might take a look at the suggestion in
http://mail.python.org/pipermail/python-list/2006-March/375280.html
to get SEEK_SET, etc.
action/license.py:
- line 108: why are you doing this? Ask for forgiveness, etc. I'd say
the same about the size calculations in generic.py, but I see how that
would get very indented (I'd still suggest you do it that way, though;
perhaps splitting clauses out into separate functions).
publish/transaction.py:
- line 114: Why is this necessary? Can't you just use __repo_cache?
- line 130: are there any issues with multiple simultaneous publishers?
(Not necessarily just here, but in general.)
- line 144: no need for str()
- line 240: you could invert this, and simply return status, "None" when
not e, and dedent the "if e" clause.
- line 386: could you be overwriting a msg that the server provided?
- line 494ff: I'd call these FileTransaction, etc.
server/catalog.py:
- line 141: os.path.basename(cmd) in ("pkg.depotd", "depot.py")? But
perhaps this should be triggered by some global variable the depot
sets (environment variable, maybe?), instead of the command name?
server/config.py:
- line 54: I'd say that if you have to tell it whether or not to create a
repo, then it isn't really "auto". "create_repo", maybe?
- line 102: we get here if the makedirs() up on line 97 failed, right?
But wouldn't an exception have been raised at that point? You can also
do
if os.path.exists(d):
if self.auto_create:
raise ...
raise ...
server/transaction.py:
- line 117: what needs to be done in init? And why can't it?
- line 120: Don't we now do this when creating the fmri object?
- line 140: why not pass the exception in directly? Aren't we doing that
now in most places?
- line 144: doesn't this mean that the args array in TransactionError is
the empty list, and we'll throw when doing args[0] on line 50?
- line 212: previously, we had a bunch of stub code here that would deal
with package completeness. Shouldn't that stay?
- line 216: what happens if this fails? Previously we'd log it and drive
on; now it throws an exception and the fact that the actual transaction
succeeded is lost.
- line 231: I'd rather see size = int(action.attrs.get("pkg.size", 0))
- line 276: I know it was this way before, but how does this raise a
KeyError? I could see how line 273 could.
- line 357: I'm not sure why the comment that was previously here about
multiple packages moving to PUBLISHED at the same time was removed.
server/repository.py:
- line 60, 65, 260: Ditch "SHA-1" -- there's no reason this exception
should be tied to the hash we're using.
- line 79: should match line 72 in structure. Doesn't matter which way,
really. I'd probably lean toward 79, but I might even go with the more
terse "The specified XXX '%s' is invalid.", which might be too terse
for your tastes. Line 110, too.
- line 96: should this have a line break in here somewhere?
- line 146: again, why the test? Just try to create it. If it fails,
create one from scratch.
- line 150: no need for parens around cfgpathname.
- line 176, et al: strike "in-flight".
- line 206: "Generates ..." rather than "Returns a generator object".
- line 214: would "if isinstance(last_modified, basestr):" be better?
- line 224: no need to split the line.
- line 242: "Closes the transaction specified by 'trans_id'."
- line 261: probably should have a blank line before it. But doe these
calls belong in repository? Or should they go in depot? I have the
same feeling about the search-related stuff (well, maybe indexing makes
sense here).
- line 315: if open() isn't going to come before the other transaction
methods, then it should be in alphabetical order like everything else
seems to be.
Danek
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
