On 06/ 1/10 03:09 AM, Tim Foster wrote:
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.
No, it's really 80.
...
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?
...
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.
...
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
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.
Otherwise, LGTM.
Cheers,
-Shawn
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
