Re: [Monotone-devel] automate log

[Stephen Leake]

Thomas Keller <address@hidden> writes:

> Am 06.07.2010 09:28, schrieb Stephen Leake:
>> Thomas Keller <address@hidden> writes:
>>> 3) in cmd_diff_log.cc:
>>>
>>>   +void
>>>   +log_print_rev (app_state &      app,
>>>   +               database &       db,
>>>   +               project_t &      project,
>>>   +               revision_id      rid,
>>>   +               revision_t &     rev,
>>>   +               string           date_fmt,
>>>   +               node_restriction mask,
>>>   +               bool             automate,
>>>               ^^^^
>>>
>>>   Please use an enum for this and later uses.
>> 
>> I prefer a bool, for a couple reasons:
>> 
>> - I'm not clear what the other name should be (note your example below
>>   doesn't give one). "automate" and "not automate" are clear in this
>>   context.
>
> Just because I didn't give you the other option, this shouldn't hold you
> off finding a good name - f.e. "user_log".

I find "bool automate" to be perfectly clear. Finding another name for
the other option is a waste of time and energy (admittedly not a big waste).

"user_log" would be very bad; it should be a name that means the same
thing in all other automate/user commands, since they will (in general)
need this same option. Finding _good_ names is not easy!

See? We've already wasted more time than it's worth :).

> The reason why I (and I think a couple of other people here as well)
> hate boolean function arguments with a passion is that they make it
> harder to understand what the meaning of the function argument actually
> is. 

Yes. You have to go look at the function spec to find the name of the
argument. That's what named association is for in more modern languages. 

It doesn't help that g++ doesn't output decent cross referencing info,
so Emacs can't jump to the spec automatically.

The Gnu Ada compiler does output cross references; that's two reasons to
prefer Ada :).

> This gets extra hard if there is not just one, but a couple of boolean
> function arguments - and since you can't force people to write
>
>   bool first_arg = true, second_arg = false;
>   my_func_call(first_arg, second_arg);
>
> - which looks a little stupid to me anyways - its better to use an enum
> value right away.

With more realistic names instead of 'first_arg', 'second_arg', this
would look better.

This issue is not limited to bool; any time there are several args of the
same type, confusion is likely. 

With Ada named association, this call would be:

    my_func_call(first_arg => true, second_arg => false);
 
I try to use this commenting style to label the args when there is more
than one of the same type:

    my_func_call(true,   // first_arg
                 false); // second_arg

I could do the same for single bool args; that would be ok.

So I see your proposed use of an enum type as a workaround for the lack
of named association in C++. In general, I hate workarounds like this; I
prefer to expose the problems as part of advocating for better language.
But I'm really not proposing to rewrite mtn in Ada :).

But I do prefer my commenting style workaround to using enums instead of
bools; it's more general, and saves the effort of making up names.

>> - When there's an enum, I have to wonder how many choices there are, so
>>   case statements are required. That seems overkill for two choices.
>>   Your example uses an 'if', so a bool is more appropriate.
>
> It makes the code much more describable.

Apparently we disagree on this. Can you give an example of describing
this code? Here's one:

    The 'automate' flag controls what is output; when true, only the
    revision id is output. When false, the author, date, and changelog
    are output; the non-automate output is further controlled by other
    options. 

Using 'target' and 'user_log':

    The 'target' flag controls what is output; when 'automate', only the
    revision id is output. When 'user_log', the author, date, and changelog
    are output; the non-automate output is further controlled by other
    options. 

I find the first more understandable (but not by much). In the second, I
have to wonder what 'target' means, and why the non-automate version
seems to be specific to 'log', rather than general to all non-automate
commands.


Perhaps 'bool automate_p' would be better? Where '_p' means 'predicate'?
That's a common convention in lisp code.

>> There are certainly many other places in mtn that use bool for similar
>> choices.
>
> Yes, unfortunately, and my mission is to change this in the future :)

Sometimes I like tilting at windmills, too :).

I'll merge this to main soon.

Thanks again.

-- 
-- Stephe