Re: [Monotone-devel] automate log

[Thomas Keller]

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".

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. 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.

> - 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.

> 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 :)

>>   Also, because log_print_rev is only used twice and the use is
>>   determined with this
>>
>>   if (automate)
>>     log_print_rev (app, db, project, rid, rev, date_fmt, mask, \
>>                    automate, output);
>>   else
>>     ostringstream out;
>>     log_print_rev (app, db, project, rid, rev, date_fmt, mask, \
>>                    automate, out);
>>
>>   The code could be simplified like this:
>>
>>   if (target == automate_log)
>>     out << rid << "\n";
>>   else
>>     {
>>        // the former code which prints everything else ...
>>     }
> 
> Yes, that is cleaner. But I'll keep log_print_rev for the else branch;
> that makes it easier to see the logic flow. Comparing either version to
> main requires telling ediff to focus on regions.

Ok.

>> 4) You have quite a lot whitespace changes
> 
> The only way to fully eliminate whitespace changes is for every file to
> follow a standard convention. I have emacs set to enforce what I
> perceive to be the monotone convention, so it fixes files that I edit.
> 
> It's easy to ignore whitespace changes in a good diff tool, so I don't
> see this as a problem.

Yes, this is not a strong objection, I tend to fix whitespaces
"on-the-run" as well, so I shouldn't throw around bricks in the glass
house :), but still, unless someone comes around and implements
--no-whitespace in mtn diff, we should still take a little care of that.

> Thanks for your review. Changes synced.

Thanks for your work.

Thomas.

-- 
GPG-Key 0x160D1092 | address@hidden | http://thomaskeller.biz
Please note that according to the EU law on data retention, information
on every electronic information exchange might be retained for a period
of six months or longer: http://www.vorratsdatenspeicherung.de/?lang=en

signature.asc
Description: OpenPGP digital signature