Derrick Stolee <stolee@xxxxxxxxx> writes:
>> +static const char turn_off_instructions[] =
>> +N_("\n"
>> +"Turn this message off by running\n"
>> +"\"git config %s false\"");
>
> I have mixed feelings on the use of these instructions. Perhaps at
> minimum the addition of these instructions could be left to a
> separate patch than the creation of advise_ng().
>
> My biggest concern is that this adds unexpected noise to users who
> want the advice to stay. I'm calling attention to it, because this
> part isn't a simple refactor like the rest of the patch.
> ...
> I definitely tend to recommend more tests than most, but perhaps this
> unit test is overkill? You demonstrate a good test below using a real
> Git command, which should be sufficient. If the "turn this message off"
> part gets removed, then you will still have coverage of your method.
> It just won't require a test change because it would not modify behavior.
> ...
All good suggestions. Thanks for an excellent review.
Another thing.
advise_ng() may have been a good name for illustration but is a
horrible name for real-world use (imagine we need to revamp the API
one more time in the future---what would it be called, which has to
say that it is newer than the "next generation"?
advise_3rd_try()?).
Thanks.
