2008/7/9 Danek Duvall <[EMAIL PROTECTED]>: > At > > http://cr.opensolaris.org/~dduvall/pkg-newlist/
http://cr.opensolaris.org/~dduvall/pkg-newlist/src/client.py.wdiff.html ========== 821 +pkg: no packages matching the following patterns you specified are 822 +installed on the system. Specify -r to retrieve requested information 823 +from the repository:""")) Won't -r retrieve information from more than one possible repository if they more than one authority (in the future, if not now)? http://cr.opensolaris.org/~dduvall/pkg-newlist/src/modules/catalog.py.wdiff.html ========== 193 + does, it will be ignored. 194 + """ Shouldn't line 194 be moved up to line 193 to match line 216, etc.? 204 + else: 205 + if version not in d[fmri.pkg_name]: 206 + d[fmri.pkg_name][version] = \ 207 + (fmri, [ auth ]) 208 + bisect.insort( 209 + d[fmri.pkg_name]["versions"], fmri.version) 210 + else: 211 + d[fmri.pkg_name][version][1].append(auth) Couldn't 204 be changed to elif merging 204 and 205 and bringing 210 back an indent level? http://cr.opensolaris.org/~dduvall/pkg-newlist/src/modules/client/image.py.wdiff.html ========== 654 + os.symlink("../../pkg/%s/installed" % fmri.get_dir_path(), 655 + "%s/state/installed/%s" % (self.imgdir, fmri.get_link_path())) 666 + if not os.path.isdir("%s/state/installed" % self.imgdir): 670 + os.unlink("%s/state/installed/%s" % (self.imgdir, 671 + fmri.get_link_path())) Why not os.path.join here for the various paths and store installed path and installed.builed path in variables? Are we not supposed to always use it? 920 + self.cache_catalogs() 921 + 894 922 if failed: 895 923 raise RuntimeError, (failed, total, succeeded) Is 920 and 921 intentionally before 922/923? Is the thought that we should cache what we can? Is there any concern that if failed was true that we would be caching an inconsistent or bad catalog state? 1067 + self.pkg_states[fmristr] = ("installed", f) 1085 + self.pkg_states[fmristr] = ("installed", f) 1288 + "state": inst_state, 1289 + "upgradable": ver != newest, 1290 + "frozen": False, 1291 + "incorporated": False, 1292 + "excludes": False It would be nice to have constants for the pkg states instead of hardcoding "installed", etc. in various places. 1192 + patterns = [ ] Space between []. http://cr.opensolaris.org/~dduvall/pkg-newlist/src/modules/fmri.py.wdiff.html ========== 275 + return "%s" % (urllib.quote(self.pkg_name, "")) Why not just return "urllib.quote(self.pkg_name, "")" ? General Comments ========== I tried out your patch. list is *much* better than it used to be for the -s and -a case. *However*, I noticed that "list -sa" is still atrociously slow. Still, on my local system, after the first run, list -a completes in seconds instead of minutes as it does now. Your changes look fine otherwise. Cheers, -- Shawn Walker _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
