Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX: Use standard error for die(), dieWithHelp(), and error messages #404

Merged
merged 2 commits into from
Sep 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions tests/t1200-pri.sh
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ TODO: 2 re-prioritized from (C) to (A).
TODO: 3 of 3 tasks shown

>>> todo.sh pri 2 a
=== 1
2 (A) notice the sunflowers
TODO: 2 already prioritized (A).

Expand Down
1 change: 1 addition & 0 deletions tests/t1500-do.sh
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ test_todo_session 'fail multiple do attempts' <<EOF
TODO: 3 marked as done.

>>> todo.sh -a do 3
=== 1
TODO: 3 is already marked done.
EOF

Expand Down
1 change: 1 addition & 0 deletions tests/t1700-depri.sh
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ test_todo_session 'depriority of unprioritized task' <<EOF
TODO: 3 of 3 tasks shown

>>> todo.sh depri 3 2
=== 1
TODO: 3 is not prioritized.
2 notice the sunflowers
TODO: 2 deprioritized.
Expand Down
3 changes: 3 additions & 0 deletions tests/t1800-del.sh
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ test_todo_session 'del with confirmation' <<EOF
TODO: 3 of 3 tasks shown

>>> printf n | todo.sh del 1
=== 1
Delete '(B) smell the uppercase Roses +flowers @outside'? (y/n)$SPACE
TODO: No tasks were deleted.

Expand All @@ -71,10 +72,12 @@ TODO: No tasks were deleted.
TODO: 3 of 3 tasks shown

>>> printf x | todo.sh del 1
=== 1
Delete '(B) smell the uppercase Roses +flowers @outside'? (y/n)$SPACE
TODO: No tasks were deleted.

>>> echo | todo.sh del 1
=== 1
Delete '(B) smell the uppercase Roses +flowers @outside'? (y/n)$SPACE
TODO: No tasks were deleted.

Expand Down
1 change: 1 addition & 0 deletions tests/t1910-deduplicate.sh
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ EOF

test_todo_session 'deduplicate without duplicates' <<EOF
>>> todo.sh deduplicate
=== 1
TODO: No duplicate tasks found
EOF

Expand Down
6 changes: 3 additions & 3 deletions tests/t2120-shorthelp.sh
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ echo 'export TODO_ACTIONS_DIR=$HOME/custom.actions' >> custom.cfg
export TODOTXT_GLOBAL_CFG_FILE=global.cfg

test_todo_session '-h and fatal error without config' <<EOF
>>> todo.sh -h | sed '/^ \\{0,2\\}[A-Z]/!d'
>>> todo.sh -h 2>&1 | sed '/^ \\{0,2\\}[A-Z]/!d'
Usage: todo.sh [-fhpantvV] [-d todo_config] action [task_number] [task_description]
Actions:
Actions can be added and overridden using scripts in the actions
Expand All @@ -58,7 +58,7 @@ EOF

# Config option comes too late; "Add-on Actions" is *not* mentioned here.
test_todo_session '-h and fatal error with trailing custom config' <<EOF
>>> todo.sh -h -d custom.cfg | sed '/^ \\{0,2\\}[A-Z]/!d'
>>> todo.sh -h -d custom.cfg 2>&1 | sed '/^ \\{0,2\\}[A-Z]/!d'
Usage: todo.sh [-fhpantvV] [-d todo_config] action [task_number] [task_description]
Actions:
Actions can be added and overridden using scripts in the actions
Expand All @@ -69,7 +69,7 @@ EOF

# Config option processed; "Add-on Actions" is mentioned here.
test_todo_session '-h output with preceding custom config' <<EOF
>>> todo.sh -d custom.cfg -h | sed '/^ \\{0,2\\}[A-Z]/!d'
>>> todo.sh -d custom.cfg -h 2>&1 | sed '/^ \\{0,2\\}[A-Z]/!d'
Usage: todo.sh [-fhpantvV] [-d todo_config] action [task_number] [task_description]
Actions:
Actions can be added and overridden using scripts in the actions
Expand Down
27 changes: 18 additions & 9 deletions todo.sh
Original file line number Diff line number Diff line change
Expand Up @@ -349,14 +349,14 @@ dieWithHelp()
case "$1" in
help) help;;
shorthelp) shorthelp;;
esac
esac >&2
shift

die "$@"
}
die()
{
echo "$*"
echo >&2 "$*"
exit 1
}

Expand Down Expand Up @@ -1170,7 +1170,7 @@ case $action in
echo "TODO: $item deleted."
fi
else
echo "TODO: No tasks were deleted."
die "TODO: No tasks were deleted."
fi
else
sed -i.bak \
Expand Down Expand Up @@ -1200,6 +1200,7 @@ case $action in

# Split multiple depri's, if comma separated change to whitespace separated
# Loop the 'depri' function for each item
status=0
for item in ${*//,/ }; do
getTodo "$item"

Expand All @@ -1211,9 +1212,11 @@ case $action in
echo "TODO: $item deprioritized."
fi
else
echo "TODO: $item is not prioritized."
echo >&2 "TODO: $item is not prioritized."
status=1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it necessary here to leave the whole process with failure exit status? Maybe you could test if other $items succeed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me, failure is the inability to do what I was told, even if I partially succeeded. The user still has the action's output to see which items succeeded and which didn't; the exit status is more of interest from a scripting perspective; the shell doesn't visualize the exit status by default (but I color the following prompt red if there was an error). And from that scripting perspective, it's more interesting whether the whole command succeeded, not whether it got partially done. (And you can always use a loop and invoke the command with each item separately to obtain that information.)

fi
done
exit $status
;;

"do" | "done" )
Expand All @@ -1224,6 +1227,7 @@ case $action in

# Split multiple do's, if comma separated change to whitespace separated
# Loop the 'do' function for each item
status=0
for item in ${*//,/ }; do
getTodo "$item"

Expand All @@ -1239,15 +1243,17 @@ case $action in
echo "TODO: $item marked as done."
fi
else
echo "TODO: $item is already marked done."
echo >&2 "TODO: $item is already marked done."
status=1
fi
done

if [ $TODOTXT_AUTO_ARCHIVE = 1 ]; then
# Recursively invoke the script to allow overriding of the archive
# action.
"$TODO_FULL_SH" archive
"$TODO_FULL_SH" archive || status=$?
fi
exit $status
;;

"help" )
Expand Down Expand Up @@ -1363,7 +1369,7 @@ case $action in
echo "TODO: $item moved from '$src' to '$dest'."
fi
else
echo "TODO: No tasks moved."
die "TODO: No tasks moved."
fi
;;

Expand All @@ -1374,6 +1380,7 @@ case $action in

"pri" | "p" )
shift
status=0
while [ "$#" -gt 0 ] ; do
item=$1
newpri=$( printf "%s\n" "$2" | tr '[:lower:]' '[:upper:]' )
Expand Down Expand Up @@ -1405,10 +1412,12 @@ note: PRIORITY must be anywhere from A to Z."
fi
fi
if [ "$oldpri" = "$newpri" ]; then
echo "TODO: $item already prioritized ($newpri)."
echo >&2 "TODO: $item already prioritized ($newpri)."
status=1
fi
shift; shift
done
exit $status
;;

"replace" )
Expand Down Expand Up @@ -1476,7 +1485,7 @@ note: PRIORITY must be anywhere from A to Z."
newTaskNum=$( sed -e '/./!d' "$TODO_FILE" | sed -n '$ =' )
deduplicateNum=$(( originalTaskNum - newTaskNum ))
if [ $deduplicateNum -eq 0 ]; then
echo "TODO: No duplicate tasks found"
die "TODO: No duplicate tasks found"
else
echo "TODO: $deduplicateNum duplicate task(s) removed"
fi
Expand Down