On Tue, Sep 3, 2024, at 6:11 PM, Elijah Newren wrote: > On Tue, Sep 3, 2024 at 12:43 PM Zack Weinberg > <zack@xxxxxxxxxxxx> 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. >> 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. >> 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? >> 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
