Allow parameters for todo.sh default action #407
Conversation
|
Unfortunately, you're written the test to match the implementation, not to actually verify the specified behavior; and that actually was broken (good that we insisted on having tests). I've corrected both in my commit. It still doesn't handle default parameters that have spaces or other special characters; I'll add that later. |
|
Thanks for your help! What special parameters for example do you mean that should be tested? |
|
@inkarkat I would like to relieve you of implementing the tests if ou give me some hints. |
… by the action The foo2 action needs to print its parameters (minus the first which is the action name, instead of hard-coding "foo" if no parameters are passed (which also is broken because the $# is already expanded in the here-doc)), and the test needs to include "foo" (instead of "bar") to match the (reused) expected output.
inkarkat
force-pushed
the
allow-params-default-action
branch
from
4622dc5
to
780c114
Compare
…tion Spaces, quotes, $, and other special shell characters need to be escaped inside the TODOTXT_DEFAULT_ACTION, and consequently eval() used to parse that back into individual arguments.
Only $TODO_ACTIONS_DIR/$action, not even the $TODO_ACTIONS_DIR/$action/$action variant. A totally different approach is needed: Instead of parsing off the first argument and treating that as the custom action, do a recursive invocation with the default action parsed as a command-line. A new isDefaultAction flag is needed so that we only do this when the default action actually applies (otherwise, strange invocations like todo.sh "add +project here" would also be possible). The condition for non-empty $TODOTXT_DEFAULT_ACTION avoids an endless loop.
inkarkat
force-pushed
the
allow-params-default-action
branch
from
780c114
to
e71eb46
Compare
|
Thanks for the review @chrysle; the original implementation was woefully incomplete and wrong, but only writing the tests showed what looked quite alright to a casual review. From my point of view this is now done, and we just have to wait for the merge of the huge backlog of open issues... |
|
Did you notice the test for macOS 10.15 succeeded? I wonder why it was always cancelled before. |
Before submitting a pull request, please make sure the following is done:
master.fixes #XXreference to the issue that this pull request fixes.This finishes leftover work from #159
That PR can be closed then.
I added a test, a docstring and also a note for
TODOTXT_DEFAULT_ACTIONto the config file.