> In these new commands the syntax seems to be different. Why is that?
Stephen picked the command syntax. I implemented it.
> Question 2: Could we see some sample output?
Sure.
$ pkg authority
AUTHORITY URL
awd (preferred) http://awd:10000/
mumak http://mumak:10000/
$ pkg authority awd mumak
Authority: awd
Origin URL: http://awd:10000/
SSL Key: None
SSL Cert: None
Catalog Last Updated: 2008-02-21 13:08:40.860697
Authority: mumak
Origin URL: http://mumak:10000/
SSL Key: None
SSL Cert: None
Catalog Last Updated: 2008-04-02 00:01:58.368630
> I mostly reviewed client.py and the test cases-- didn't do thorough coverage
> of the rest.
>
> client.py:
>
> 866 + print \
> 867 + "pkg: set-authority: one and only one authority may be
> set"
> 868 + usage()
>
> Pass this string as a parameter to usage(), don't forget to wrap it in _().
> Note: a lot of other strings you are adding are missing _()... I tried to
> clean this up in my recent wad.
Okay. What does _() do, why do my strings need to be wrapped in this,
and where can I read more about how this works?
> 874 + if not os.path.exists(ssl_key):
> 875 + print _("pkg: set-authority: SSL key file '%s' does
> not exist"
> 876 + ) % ssl_key
>
> Use error() instead, it ensures that output goes to stderr and does
> the pkg: prefixing for you. (also applies to a number of other places)
I think I got most of these when I dealt with Danek's comments. I'll
double-check.
>
> 925 + try:
> 926 + opts, pargs = getopt.getopt(args, "H")
> 927 + for opt, arg in opts:
> 928 + if opt == "-H":
> 929 + omit_headers = True
> 930 + except getopt.GetoptError, e:
> 931 + print "pkg: authorities: illegal option -- %s" %
> e.opt
> 932 + usage()
>
> IIRC the exception handling is not needed-- if you simply leave the exception
> uncaught, it'll get caught in main_func(). (ditto for line 861)
Thanks, I'll fix that.
> 940 + if pfx == preferred_authority:
> 941 + pfx += " (preferred)"
> 942 + print "%15s %30s" % (pfx, url)
> 943 + else:
> 944 + print "%15s %30s" % (pfx, url)
>
> Given that " (preferred)" is 12 characters in length, is the first column wide
> enough?
I had no idea what the right choice was here, so I guessed. If you'd
like to suggest more sane options, I'd certainly take that advice.
> 959 + print "\nAuthority: %s" % pfx
> 960 + print "Origin URL: %s" % url
> 961 + print "SSL Key: %s" % ssl_key
> 962 + print "SSL Cert: %s" % ssl_cert
> 963 + print "Catalog Last Updated: %s" % dt
>
> Consider copying the output style of 'pkg info'?
No. What aspects were you interested in seeing imitated?
> 1013 + ssl_key = os.path.abspath(ssl_key)
>
> Why abspath()? (os.path.exists() returns false for broken symlinks).
It's a present from Stephen. Is os.path.exists() the approved way of
doing this?
> t_commandline.py:
>
> Please extend test_pkg_missing_args, and test_pkg_bogus_opts for your
> changes.
>
> I'd also like to see a more thorough vetting of the functionality
> in the test cases-- there's a lot of error paths you have which aren't
> being tested:
>
> SSL key specified
> SSL key specified, file doesn't exist
>
> Cert specified
> Cert doesn't exist
>
> Origin URL missing for new authority
>
> More than one authority given (i.e. the error message on line
> 864 of client.py)
>
> Attempted removal of preferred authority
>
> Listing a specific authority
> Listing a specific authority which does not exist
I may end up doing this in a separate putback, depending upon how
pressed we are for time. Thanks for the throrough and specific list of
tests to include.
-j
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
