Re: [PATCH] advice: refactor advise API

> Derrick Stolee <stolee@gmail.com> 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()?).