Re: [PATCH] advice: refactor advise API
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 10, 2020, 19:30 UTC
- Message-ID
- <xmqq8slaz1cp.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <97406f9e-b8ef-b5b9-3987-cdef843b31a5@gmail.com>
Derrick Stolee <stolee@gmail.com> writes:
Show 19 quoted lines
>> +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.