Hi Shawn, Thanks for taking a look - it's much appreciated.
On Mon, 2010-05-31 at 16:32 -0700, Shawn Walker wrote: > > http://cr.opensolaris.org/~timf/pkgdepend-smf-v3 > > src/modules/flavor/smf_manifest.py: > line 39ff: nit: s/Smf/SMF/g Yep, fixed this and the typos. > line 81: s/authoriatitive/authoritative/ > > line 107: insert additional newline > > lines 129-132: wrap at 80; can you check the rest of the file too? I thought we wrap at 90 cols? (which I always thought weird, but then the 8-space tabs is also weird ...) but I'll check that I'm wrapping at 90. > lines 177-179: simplify to: > manifests.update(search_smf_dic(fmri, instance_mf)) > > lines 181-183: simplify to: > manifests.add(search_smf_dic(fmri, > SmfManifestDependency.instance_mf)) Yep. > line 188: stray newline? > > line 194: drop the else Fixed both of these. > line 195: this seems weird; is it really ok to return any arbitrary > element of manifests? pop() won't necessarily return the last > item added to a set Yes - I'm using a set to weed out duplicates, so at this point we should only ever have a set with one element, in which case pop() here will return that single element. When support in SMF goes back to split service definitions across multiple files, more work will be needed here. > lines 317-318: s/we make/makes/ s/FMRIs/FMRIs;/ ? > line 324: actually, a tuple of None, None Yep, thanks. > src/tests/api/smf_manifests.py: > Hmmm; where's the actual unit tests? They're in t_dependencies.py, lines 975-1164. I could add the data from this module directly to t_dependencies, but thought that doing so would make that module overly verbose. > src/tests/api/t_dependencies.py: > line 760: why pass a tuple here instead of single value? That looks like a copy/paste error on my part - it's also present in the existing variants tests. I've fixed it here and elsewhere. > line 998-1001: Why assert an always False condition? I think you > really want something like: > > self.assert_(dep in deps[fmri], "%s not found in " > " dependencies for %s" % (dep, manifest)) > > line 1165: insert newline Fixed. > src/util/distro-import/importer.py: > Does this all work now when accessing a repository using 'file:' ? > It doesn't appear to given lines 749-777. Good point, I'd meant to revisit that. I've got it working now: we can do a slim_import to a file-based repository, and it will pull information about SMF manifests for that $BUILDID from the repository. I will be awfully happy when importer.py goes away. > line 1078: can you use mkdtemp() or the like instead of hard-coding > /tmp ? Yes, absolutely. > lines 1110-1114: simplify to: > return [pkg.fmri.PkgFmri(pfmri) for pfmri in fmris] > > line 1533: s/+/ + / Thanks for these. > line 1859: s/local_smf_manifests/local_smf_manifests, True) > (Since it doesn't matter if the directory doesn't exist or can't > be removed.) Yep, fair point. I've got updated webrevs with these changes at: http://cr.opensolaris.org/~timf/pkgdepend-smf-v4 http://cr.opensolaris.org/~timf/pkgdepend-smf-v4-vs-v3 cheers, tim _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
