Pass addon arguments through verbatim, as documented - #2699
Conversation
… flags The pre-cmdargs required-value check used the union of every subcommand's required-value flags, and scanned past --, so a flag belonging to some other command was rejected with "needs a value" instead of being left for cmdargs or passed on to an addon. Now the check is given the flag set relevant to the identified command (its mode's flags for a builtin, hledger's general flags for an addon), and stops at the first --, as cmdargs does. [simonmichael#2696] AI usage: claude-fable-5, ~35k output tokens
The addon argument list was rebuilt with filter (/="--"), removing every --, including ones the addon itself was meant to receive. Rebuild it from the individual argument sources instead, so that only the -- separating addon args from hledger's own is consumed, as documented. The separator is still added to the args cmdargs parses, so unknown addon flags are still accepted there. [simonmichael#2696] AI usage: claude-opus-5, ~14k output tokens
dropCliSpecificOpts removed -n/--no-conf/--conf VAL from the whole addon argument list, so an addon could not be given those even by escaping them with --. Apply it only to the args preceding the first --; pass the rest through verbatim. Without a --, they keep their current meaning as hledger's own options, which the manual now states. [simonmichael#2696] AI usage: claude-opus-5, ~14k output tokens
Addon arguments were joined into a shell command with quoteForCommandLine, which returns the empty string unquoted, so an empty argument disappeared before the addon saw it, and other quoting hazards applied. Run the addon with rawSystem instead, passing the arguments through untouched. Windows still goes through the shell, which is what runs .bat and other script addons there. [simonmichael#2696] AI usage: claude-opus-5, ~14k output tokens
f6862ac to
6bd6c66
Compare
|
Thanks @kfkonrad. This seems not to satisfy the first-time contributor rule in the https://hledger.org/PULLREQUESTS.html I mentioned. That rule was added to screen out drive-by and policy-ignoring contributors. You're clearly not one of those (right ? :) so I might change this rule. On the other hand, complicating the rule and requiring maintainer judgement every time defeats the purpose a little bit. Is there any other small PR you'd like to submit manually to become a non-first-time contributor ? |
|
Also, the rule's wording has some ambiguity.
|
|
The changes look reasonable to my eye.. though when I've done similar fixups like adding a It would be good to test this also in |
2 participants
Fixes #2696: pass addon arguments through verbatim, as documented.
As we discussed in chat and in #2696 I split the changes into four commits in the following order:
--, sohledger foo --sortandhledger help --sortfailed with "--sort needs a value". It is now given only the flags relevant to the identified command, and stops at the first--.filter (/="--")removed every--from the addon's arguments. Only the escaping one is consumed now; later ones are passed on.-n/--no-conf/--conf VALwere stripped from the whole addon argument list. They are now stripped only before the first--, sohledger foo -- -ndelivers-n. Without a--they keep their current meaning as hledger's own options; the manual says so now.rawSystemrather than a shell command line, so empty and shell-significant arguments survive.Tests: A total of 12 new cases in hledger/test/cli/addons.test and cli.test, using a new
hledger-argvdumpdummy addon (as demoed in #2696). All tests passed on my machine for each commit.Note that commit 4 only fixes the issue on unix-like systems. For Windows we still shell out to cmd, which presumably swallows empty arguments (
""). I can try to verify this on my Windows machine soon. I'm mostly using a Mac or Linux these days. So no regression on Windows but also presumably no fix. If I can verify that behavior I'll work around that too in a 5th commit.I also noticed that
hledger runmay be subject to similar bugs regarding empty or shell-significant arguments (like"'"). I may file a separate issue if I can confirm that so that heldger and hledger run behave the same when using addons. As I don't use hledger run myself I'm not sure if changing its argument parsing or dispatch might brake existing use cases.AI usage: written with some help from Claude Opus, from a plan created with Claude Fable. Some finer adjustments in the plan were my doing and I made sure to test and review each commit thoroughly. Usage in output token is denoted on each commit. btw: I chose to keep with the existing pattern of only documenting use of one model per commit, which is why the plan's AI usage is included in the first commit and the implementation is divvied up among the rest.