On Mon, Nov 18, 2013 at 1:01 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Sat, Nov 16, 2013 at 4:57 AM, Christopher Browne <cbbrowne@gmail.com> wrote:
> Few comments:
>
> 1.
> + <term><option>-g</></term>
> + <term><option>--roles</></term>
>
> All other options which require argument are of form:
> <term><option>-c <replaceable class="parameter">number</replaceable></></term>
> <term><option>--connection-limit=<replaceable
> class="parameter">number</replaceable></></term>
>
> So I think it is better to have this new option which require argument
> in similar form.
Sounds good, done.
> 2.
> + Indicates roles to associate with this role.
>
> I think word associate is not very clear, wouldn't it be better to
> mention that this new role will be member of roles specified.
> For example:
> Indicates roles to which the new role will be immediately added as a new member.
With a switch of "immediately" and "added", done. That does better
describe the behaviour.
> 3.
> + case 'g':
> + roles = pg_strdup(optarg);
> + break;
>
> If we see most of other options in case handling are ordered as per
> their order in long_options array. For example
>
> static struct option long_options[] = {
> {"host", required_argument, NULL, 'h'},
> {"port", required_argument, NULL, 'p'},
> ..
>
> Now the order of handling for both is same in switch case or while get
> opt_long() function call. I think this makes code easy to understand
> and modify.
> However there is no functionality issue here, so you can keep the code
> as per your existing patch as well, this is just a suggestion.
That is easy enough to change, and yes, indeed, having the new code
look just like what it is near seems an improvement.
I picked the location of the 'g:' in the opt_long() call basically arbitrarily;
if there is any reason for it to go in a different spot, I'd be happy to
shift it.
--
When confronted by a difficult problem, solve it by reducing it to the
question, "How would the Lone Ranger handle this?"
