Shawn Walker wrote:
> Danek Duvall wrote:
> >Shawn Walker wrote:
> >>http://cr.opensolaris.org/~swalker/pkg-cat-p3/
> ...
> >pkg.depotd.1m.txt:
> ...
>
> > - line 258: "collection-type" instead of "collection_type", et al.
> >
> > - line 268, 272: using "uri" in some places and "url" in others is
> > confusing. Also plural vs singular.
>
> Yes, they're inconsistent, and yes the '_' vs. '-' is noted. I've been
> deferring this work to the future repository configuration overhaul. The
> primary thing I was trying to do in this wad was to document them so that
> they could be used with the --property option.
Okay.
> >modules/catalog.py:
> ...
> > - line 633: Is there any way you can think of to use "key" instead of
> > "cmp"?
>
> Concatenating the stem and publisher (e.g. "%s!%s" % (stem, pub)) then
> doing the sort, and then splitting them apart again as I iterate over the
> sorted list?
That occurred to me, but I rejected it because I think I got the sense of
the test on 628 backwards. I agree that it should work. And it's supposed
to be quite a bit faster, though as K has shown us, testing it to be sure
would be a great idea. Doesn't have to be this wad.
> > - line 2201: why is this an exceptional condition, rather than just a
> > return of an empty list, as on line 2190?
>
> Line 2190 is wrong and is a holdover from when the catalog.attrs file
> was being conditionally retrieved. I've changed this.
Okay. Though part of my question is about the interface -- it doesn't seem
like an unmodified catalog is an exceptional condition, and that it really
*ought* to be returning something instead.
> >depotcontroller.py:
> >
> > - line 94: don't need the last ", None", I think.
>
> Wouldn't that raise a KeyError if the first condition returned {} ?
I think the second argument to get() defaults to None.
> >server/repository.py:
> ...
> > - line 770: this isn't safe; why not just copy old_cat_root into
> > tmp_cat_root? Or copy the contents of old_cat_root into tmp_cat_root?
>
> Isn't safe in what manner?
There's a small window, between the rmtree and the copytree, where, because
the temporary directory you created doesn't exist, someone else could
perfectly sanely create another temporary directory with the same name.
It's hardly a big deal, though -- I just mentioned it in case there was an
easy way to not require the rmtree(), which would involve either copying
old_cat_root wholesale inside of tmp_cat_root, which would probably be
okay, though you'd need to do some massaging later on, or to copy the
contents of old_cat_root inside of tmp_cat_root, which would, I think,
require an os.listdir(), though not a full walk.
> > - abandon, add, add_package, and open all follow a common pattern.
> > You might consider a decorator to do that for you.
>
> I'd prefer to defer this particular bit as I'm not keen on possibly
> breaking this (plus I've never written my own decorators so I need to
> learn the pattern first). Can I open a bug?
Feel free to defer. I'd wait on the bug until you knew you were actually
going to do it, rather than opening it just to experiment, decide you
won't, and close it. But it's up to you how you manage your todo (or
tothinkabout) list.
> >server/transaction.py:
> >
> > - line 177: can you pass an fmri like "pkg:/SUNWcs" to the depot? If
> > so, then this could result in "pkg://opensolaris.org/pkg:/SUNWcs".
>
> I feel dumb here. If you passed 'pkg:/SUNWcs', the FMRI would pass
> the "not startswith('pkg:/')" test and would replace 'pkg:/' instead
> of pre-pending. Can you explain?
"would not pass", maybe? But yes, I think I may have missed the "not" on
that line.
> >extra_privs:
> >
> > - is this change really part of this wad?
>
> Hmm, this got picked up in a merge somehow. I don't know why it
> thinks I added this file, since that's clearly part of changeset
> 1393. Looking at the Mercurial log though, it thinks I added it as
> well. How to fix?
hg rm, I think.
Danek
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
