Re: [PATCH] advice: refactor advise API

From: Taylor Blau <me@ttaylorr.com>
To: Heba Waly <heba.waly@gmail.com>
Cc: Taylor Blau <me@ttaylorr.com>, Junio C Hamano <gitster@pobox.com>,
	Derrick Stolee <stolee@gmail.com>,
	Heba Waly via GitGitGadget <gitgitgadget@gmail.com>,
	Git Mailing List <git@vger.kernel.org>
Subject: Re: [PATCH] advice: refactor advise API
Date: Wed, 12 Feb 2020 12:57:25 -0800	[thread overview]
Message-ID: <20200212205725.GF4364@syl.local> (raw)
In-Reply-To: <CACg5j26ROdUAqCNPMRoORweEEDaUyeZvSPKS0bYLt9qYART4bA@mail.gmail.com>

On Tue, Feb 11, 2020 at 01:08:48PM +1300, Heba Waly wrote:
> On Tue, Feb 11, 2020 at 8:42 AM Taylor Blau <me@ttaylorr.com> wrote:
> >
> > On Mon, Feb 10, 2020 at 11:30:46AM -0800, Junio C Hamano wrote:
> > > 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()?).
>
> As I mentioned earlier, this patch is meant to be used as a transition
> between the current advise() and the refactored one.

Ah, thanks for pointing it out, and I'm sorry that I missed reading it
in my first review. Your idea sounds quite good to me, and thanks for
working on this.

> So the name is just temporary and it'll be renamed to advise() once
> the transition is done.
> But if we want to keep both functions, or want a better name because
> it's an open-source project and the author might not complete the
> transition, then I'll try to think of another name.
>
> > What about calling this new API 'advise()'? The first patch could call
> > it 'advise_ng' or whatever other temporary name we feel comfortable
> > using, and then each subsequent patch would update callers of 'advise()'
> > to use 'advise_ng()'. Once those patches have been applied, and no other
> > callers of 'advise()' exist, a final patch can be applied on top to
> > rename 'advise_ng()' to 'advise()', and update the names of all of the
> > callers.
> >
>
> Yes, that's what I would like to do.
>
> > This makes for a rather noisy final patch, but the intermediate states
> > are much clearer, and it would make this series rather self-contained.
> >
> > On the other hand, having a version of 'advise_ng()' on master makes
> > this topic more incremental, meaning that we can pick it up and put it
> > down at ease and have more self-contained projects.
> >
> > I don't really have a preference between the two approaches, but if we
> > go with the latter, I do think we need something better than
> > 'advise_ng'. Maybe 'advise_warn'? I don't know.
> >
> > > Thanks.
> >
> > Thanks,
> > Taylor
>
> Thanks,
> Heba

Thanks,
Taylor