Hi Shawn, Thanks for getting the review for this out so quickly!
On Tue, Jul 13, 2010 at 05:22:42PM -0700, Shawn Walker wrote: > On 07/13/10 11:53 AM, [email protected] wrote: > >The webrev is here: > > > > http://cr.opensolaris.org/~johansen/webrev-xpub-1/ > > ====================================================================== > src/modules/misc.py: > ====================================================================== > lines 30, 37, 52, 47, 41: httplib, urlhelpers, global_settings, > urllib2, socket are all unused now Thanks, I forgot to clean up the imports on this file. This also means that I can remvoe the urlhelpers module entirely. > ====================================================================== > src/modules/publish/transaction.py: > ====================================================================== > line 31: os import not used Fixed, thanks. > ====================================================================== > src/publish.py: > ====================================================================== > line 54: this is already imported on line 50? Not sure how that happened. > lines 289, 389: not your fault, but can you kill the semicolons? Removed. > line 499: not your fault, but can you fix indentation? (off by 1) Fixed. > ====================================================================== > src/pull.py: > ====================================================================== > lines 48, 49: ptf and portable imports no longer used Removed, thanks. > ====================================================================== > src/modules/client/transport/repo.py: > ====================================================================== > line 598: missing spaces after '{' and before '}'. Also, it seems > like you could trivially add support for add to catalog here, by > (adding) and checking for X-IPkg-Add-To-Catalog. I added the spaces, but I'm confused about the rest of this comment. I re-implemented the transport parts here based upon what was in FileTransaction and HTTPTransaction. The HTTPTransaction class didn't support the add to catalog method. Are you saying that it should? Do I need to make depot changes to enable this? > line 600: Why not simply headers.update(header) ? Wasn't aware of this method. Thanks. > line 636: extra newline Fixed. > lines 719-724: Need docstring bits for 'frepo' ? Updated. > line 1154: s/content to/an action and its payload (if applicable) > to an existing transaction in/ ? Ok. > line 1165: s/'abandon' publication operation, that/abandon operation/ Changed. > line 1197: s/Returns a transaction-ID/Returns the identifier > string for the transaction/ That's a mouthful. How about, "Returns the trasaction-ID string?" > ====================================================================== > src/modules/client/transport/transport.py: > ====================================================================== > lines 85, 123: insert newline here (two lines between classes) Fixed. > line 125: missing docstring Added. > line 127: s/misc.ImmutableDict()/misc.EmptyDict/ Changed. > lines 142-143: This can be: > return (p for p in self.__publishers.values()) Ok, but doesn't that make it less obvious that this is a generator, since the parens are used in place of yield? > lines 146-148: This can be: > return self.__policy_map.get(policy_name, False) Changed. > lines 1779-1783: simplified to the following? > > repolist = repo.origins[:] > if not origin_only: > repolist.extend(repo.mirrors) Changed. This works for the __chunk_size case, but for gen_repos we actually explicitly want to put mirrors first, just in case all entries are equal. FWIW, this code isn't actually changed. Take a look at the Sdiff to be certain. > line 1961: extra newline Removed. > line 2051: so why cycle through origins for publication operations? > it seems like we should never have more than one origin for > publication. Same for other publication opeartions on 2096, etc. The __gen_origins code is also responsible for retrying at repositories where there are failures. This is used so that we retry a destination more than once. Multiple destinations probably isn't a good idea. > line 2474: list of files in filelist? Ancient copy/paste error, I think. > ====================================================================== > src/tests/cli/t_publish_api.py: > ====================================================================== > line 88: extra newline? Fixed. > ====================================================================== > src/util/distro-import/importer.py: > ====================================================================== > line 23: Should really be 2009, 2010 I believe. Changed. > lines 43, 54: repository and urlparse imports no longer used Removed. > line 560: strictly speaking, 'global' isn't required if you're only > reading global variables; just assigning (see 'pydoc global'). > This is done in other places too... Ah, I misunderstood how this works. I'll make a pass through this module, and others, to see if I can clean this up a bit. > line 1406: replace 'lib/pkg.depotd' with 'bin/pkgrepo'; also update > docstring on line 1404 > > line 1407: replace with: > argstr = "%s -s %s refresh" % (cmdname, path_to_repo) These two are non-trivial changes to validate. It took me a full day just to get the importer working to the point where I could run a test. Since I've already tested this with pkg.depotd, I'd prefer not to change this since I would have to re-test it. That takes hours to get set up. > line 1468: this comment should probably move up to above 1456 or be > changed slightly Moved. > not your fault, but can you fix these? > line 1428: indentation off by one Changed. > ====================================================================== > src/util/publish/merge.py: > ====================================================================== > lines 182-231: The code in these seems common to our publication > utilities. Any way to centralise these somewhere? (misc? a new > module? perhaps pkg.util?) I realise the globals make this > difficult. There have been some subtle variations in publisher setup between merge, importer, and pull. I thought about commonizing this code, but I was concerned that it would lead to a weirder method shared amongst them all. > not your fault, but can you fix these? > line 41: ptf import no used > > line 89: missing spaces around '=' > > lines 237, 251: indentation off by one > > line 403: missing space after ',' Fixed all of these. Thanks again for looking at this! -j _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
