Re: [filter-repo PATCH]: add --callbacks option to load many callbacks from one file

From: Zack Weinberg <zack@owlfolio.org>
Date: Sep 4, 2024, 15:13 UTC
Message-ID: <7cfe4a14-c319-4ccd-9c1d-099139a88913@app.fastmail.com>
In-Reply-To: <CABPp-BFqbiS8xsbLouNB41QTc5p0hEOy-EoV0Sjnp=xJEShkTw@mail.gmail.com>

On Tue, Sep 3, 2024, at 6:11 PM, Elijah Newren wrote:

> On Tue, Sep 3, 2024 at 12:43 PM Zack Weinberg
> <zack@owlfolio.org> wrote:
>> If you are trying to do something complicated with filter-repo,

...

> If you want to do something complicated with filter-repo, you can just
> import it as a library.

I should have said up front that I see this proposed feature as filling a hole in the power/convenience tradeoff space, between the individual --foo-callback switches and using filter-repo as a library. It facilitates doing things that are a little bit too hard if you want to use individual switches, but not so hard that it feels worthwhile to start reading the "using filter-repo as a library" examples and braving the internal API stability warnings.

In particular, it gives you the programming environment that you're accustomed to if you're an experienced Python coder -- "this file will be parsed as a Python module" is much more "normal" to such people than "this file will be parsed as the body of a function" -- but stays within the lines of the official stable callback API.

It also works without setting up the _capability_ to use filter-repo as a library, so that saves at least one preparatory step.

Show 12 quoted lines

>> Or, if you are trying to do something simple but long-winded, such as
>> replacing the entire contents of a file, you might want to define
>> long multi-line (byte) strings as global variables, to avoid having
>> to deal with the undocumented number of spaces inserted at the
>> beginning of each line by the callback parser.
>
> Yeah, I can see how the added spaces would be slightly annoying for
> the case of multi-line strings (though simple callbacks like `--name-
> callback 'return name.replace(b"Wiliam", b"William")'` require that
> some kind of leading whitespace be added, and the command line --*-
> callback options are targetted towards the simpler usecases, after
> all).  However, even in that case you can just use textwrap.dedent.

textwrap.dedent is great, but you have to know that it exists. If you're staring at the filter-repo manpage and trying to figure out how to replace a multi-line string and you haven't memorized the entire set of capabilities of the Python stdlib, "oh hey I can use --callbacks and then I can put big strings in global variables" is an easier cognitive stretch than _either_ "oh hey I can use textwrap.dedent" or "I guess I gotta figure out how to use this unstable library API that's only vaguely touched on in the manpage".

I should also mention that I started developing this feature before I knew about the possibility of passing a file name to --foo-callback. (The present state of play is that this is documented in --help but not in the manpage, and I was only looking at the manpage until I started coding.) The thought of trying to get an entire file's worth of English text through both Python and shell string quotation was too daunting to contemplate.

Show 7 quoted lines

>> The callbacks file is loaded as an ordinary Python module; it does
>> _not_ get any automatic globals, unlike individual command line
>> callbacks. However, `import git_format_repo` will work inside the
>> callbacks file, even if git_format_repo.py has not been made
>> available in general.
>
> What is this git_format_repo thing you speak of?

Doh! I meant git_filter_repo.

> Anyway, separate from that and given the above comments I made about
> importing git_filter_repo and textwrap.dedent, I'm a little unsure why
> this new callbacks command line flag helps.

I hope I have sufficiently explained the rationale above?

Show 7 quoted lines

>> Also document (briefly) the existing feature of supplying a file name
>> rather than an inline function body to --foo-callback options, and
>> the availability of an unspecified set of globals to individual
>> callbacks (with instruction to see the source code for details).
>
> Thanks for being helpful.  Do note, though, that different logical
> changes should be in separate patches.

If the overall change is approved, I can split it up into a series. I do think that the --foo-callback <filename> feature should be documented in the manpage regardless of whether --callbacks is added.

zw