Hi Shawn, Brock & Danek, Thanks for these - I've rolled up your comments into the webrev below:
http://cr.opensolaris.org/~timf/pkgdepend-smf-v5 http://cr.opensolaris.org/~timf/pkgdepend-smf-v5-vs-v4 On Tue, 2010-06-01 at 09:30 -0700, Shawn Walker wrote: > > 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. > > No, it's really 80. Ok, fixed this and updated src/tests/pylintrc to specify 80 character columns instead of 90. > ... > >> 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. > > Can you add a comment to this then just to make it clear? Done. > >> 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. > > That'd be preferred actually. We don't tend to have separate modules > for the tests. Alright, I've moved the test data into t_dependencies - I'm still not convinced it helps readability, but they're tests, so not a huge deal. > lines 1124-1126: You don't need to specify keyword arguments that > have the same value as the default. For example, the ssl_* and > origins and mirrors keywords. Thanks - fixed that. On Tue, 2010-06-01 at 13:37 -0700, Brock Pytlik wrote: >> src/tests/api/t_dependencies.py: > >> line 760: why pass a tuple here instead of single value? > Please don't change this. d.dep_key() returns a tuple. If you don't > pass d.dep_keys() inside yet another tuple, you get this error: > TypeError: not all arguments converted during string formatting > > In other words, the tuple there and other places is deliberate. Aha, I see - I've reverted that change. On Tue, 2010-06-01 at 14:32 -0700, Danek Duvall wrote: Tim Foster wrote: > > > http://cr.opensolaris.org/~timf/pkgdepend-smf-v4 > > smf_manifest.py: > > - line 92: space before close paren > > - line 159, et al: _() shouldn't include the arguments to the % > operator, or it won't be able to be translated properly. Fixed those - thanks. > dependencies.py: > > - line 232: > > pkg_attrs.setdefault(key, []).extend(attrs[key]) Ok. > importer.py: > > - line 1627ff: what are the changes here for? Good catch, sorry. That looks like a mismerge from cfb1f0e515fd on my part. I've restored those. cheers, tim _______________________________________________ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
