[pkg-discuss] Re: [review] pkg facet/variant subcommand enhancements
- From: Edward Pilatowicz <
- To: Shawn Walker <
- Subject: [pkg-discuss] Re: [review] pkg facet/variant subcommand enhancements
- Date: Mon, 10 Jun 2013 15:27:20 -0700
On Mon, Jun 10, 2013 at 03:09:37PM -0700, Shawn Walker wrote:
> On 06/10/13 14:49, Edward Pilatowicz wrote:
> >On Mon, Jun 10, 2013 at 02:21:15PM -0700, Shawn Walker wrote:
> >>On 04/19/13 14:48, Shawn Walker wrote:
> >>>The following webrev contains changes for the following items:
> >>> 15654935 pkg facet/variant should also display those implicitly set
> >>> 16425698 pkg variant subcommand should not require "variant." prefix
> >>> 16689413 facet/variant subcommands should support wildcards
> >>> 16689420 pkg facet/variant should offer parsable output format
> >>> 16689425 pkg facet/variant should have a full test suite
> >>> 16694970 manifest facets/variants properties can raise misleading
> >>> exception
> >>I've attempted to incorporate all review feedback:
> >> * facet/variant subcommand changes:
> >> - '-f' to '-a'
> >> - new '-v' option for showing possible variant values
> >> - strip 'facet.' and 'variant.' prefix from default output
> >> * more comments for change-facet/change-variant optimisations
> >> * variable naming changes
> >> * docstring updates for new functions
> >> * correctness fixes for manifest caching changes
> >> * more unit tests (unknown variant usage, etc.)
> >>Incremental webrev (diff against 1st):
> >> https://ips.java.net/webrev/srwalker/pkg-facet-variant-1-vs-2/
> >>Full webrev (all changes):
> >> https://ips.java.net/webrev/srwalker/pkg-facet-variant-2/
> >comments below.
> >- i didn't think you could have duplicate entries in opts_mapping (there
> > are now two entries using "a".)
> Reading through misc.opts_parse, and the stuff in client.py, I don't
> see how this could cause a problem. It also allows me clarity with
> the variable names.
> Also, there are actually now four entries using "a"; three of which
> I didn't add in this changeset. (The four entries are:
> li_target_all, list_available, list_installed_newest, list_all).
the rest looks good to me.