sorry for the long delay.
my comments are attached.
ed
On Tue, Jan 15, 2013 at 05:51:23PM -0800, Erik Trauschke wrote:
> Hi folks,
>
> This is a review request for the common option processing.
> It doesn't really match the original bug anymore since we changed
> the approach to this significantly. Right now it's more about
> removing the options processing out of the CLI code and into some
> common place so that other clients can use the same style of options
> as the CLI.
>
> I incorporated pretty much everything from Ed's feedback and cleaned
> out some tab indentations in client.py (in case you're wondering
> about some lines in the webrev).
>
> https://cr.opensolaris.org/action/browse/pkg/erisch/15804991/webrev/
>
> Thanks
> Erik
> _______________________________________________
> pkg-discuss mailing list
> [email protected]
> http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
https://cr.opensolaris.org/action/browse/pkg/erisch/15804991/webrev
----------
src/pkg/manifests/package:pkg.p5m
src/po/POTFILES.in
- ok
----------
src/modules/misc.py
- in opts_parse(), the name "op_opts" is kinda confusing. how about
changing this to "opts_table"? or something else?
- in opts_parse(), you removed the entire comment describing "table" but
most of this comment is still relevant / useful.
- in opts_parse(), you removed a bunch of asserts. it'd be good to have:
# make sure an option was specified
assert sopt or lopt
- in opts_parse(), you removed the asserts to check for duplicate
options in the input options table:
# make sure each key is unique
assert k not in rv
- in opts_parse(), please add back in:
# parse options
- in opts_parse(), i'm not sure i agree with this comment:
# A dict doesn't work here because some options can be passed multiple
# times. The option processing has to figure out if that's valid, this
# here should be as generic as possible.
we already know if an option can be repeated or not based on it's
"default" value. only options with a default value of an integer or a
list can be repeated. so you can check for that here. (this code
seems to have been moved to client.py right after the call to
opts_parse().
- in opts_parse(), we used to talk about option "keys", which you've
changed to option "names"
----------
src/modules/client.py
- this seems backwards:
Mapping of short and long CLI options to the internal option name.
it seems like it should say:
Mapping of internal option name to short and long CLI options.
- audit_linked(), detach_linked(), list_linked(), pubcheck_linked(),
remote(), and sync_linked() no longer check for unnecessary arguments
(ie: "len(pargs) == 0")
- why isn't all the code for building "opt_dict" inside options.py? it
seems like it should be part of opts_assemble().
- it seems to me like the "Prepare for headache" code should be in
opts_assemble(). it has all the information necessary to do this
work and future callers probably don't want to duplicate this
"headache". :)
----------
src/modules/options.py
- nit: you have some indentation that is using tabs
- nit: you have some lines that end with spaces
- nit: you have some lines that only contain whitespace (91, 334)
- nit: in __parse_linked_props() you do: "msg= _(". notice the space
after the '='. you don't do this in other places.
- you need a blank line before opts_table_li_target.
- previously, in the case of repeated options we displayed the option
and any optional argument that was repeated. now we just display the
optional argument instead and we don't display the option itself.
- in opts_table_cb_parsable(), the value for msg is not enclosed in _().
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
