[Freeipa-devel] [PATCH] enhance ipa-listdelegation

[Martin Nagy]

Rob Crittenden wrote:
> Add options to display a subset of delegations and return 2 if none
> are found.
> 
> rob

Some thoughts:

>  def parse_options():
> -    parser = OptionParser()
> +    usage = "%prog [options]"
> +    parser = OptionParser(usage)

No need for usage, "%prog [options]" is the default.

> +    parser.add_option("-s", "--source", dest="source",
> +                      help="Source group of delegation")
> +    parser.add_option("-n", "--name", dest="name",
> +                      help="Name of delegation")
> +    parser.add_option("-t", "--target", dest="target",
> +                      help="Target group of delegation")
>      parser.add_option("--usage", action="store_true",
>                        help="Program usage")

I suggest we also remove the --usage option, since it's the same as
--help if you use parser.print_help()

>      parser.add_option("-v", "--verbose", action="store_true",
> dest="verbose", @@ -56,14 +60,19 @@ def parse_options():
>      args = ipa.config.init_config(sys.argv)
>      options, args = parser.parse_args(args)
>  
> +    if options.usage or len(args) != 1:
> +        parser.print_help()
> +        sys.exit(1)
> +

This can be accomplished by parser.error("too many arguments") (without
the need for sys.exit(1).

> +        if (all or options.name == a.name or
> +            options.source == group_dn_to_cn[a.source_group] or
> +            options.target == group_dn_to_cn[a.dest_group]):

Hm, wouldn't it be better if we required that all conditions are met
in order to display them?

> +            print "Delegation Name: " + a.name
> +            print "Group " + group_dn_to_cn[a.source_group] 

Trailing whitespace (it was there before, but still :)

Martin