From 55679d136f03aaf7aa872d98aaa9d9ce2100173b Mon Sep 17 00:00:00 2001 From: Ingo Karkat Date: Sat, 17 Dec 2011 21:29:18 +0100 Subject: [PATCH 1/3] BUG: pri doesn't issue error when task does not exist. --- tests/t1200-pri.sh | 6 ++++++ todo.sh | 9 ++++++++- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/tests/t1200-pri.sh b/tests/t1200-pri.sh index 3d4ca70..7c6fe0c 100755 --- a/tests/t1200-pri.sh +++ b/tests/t1200-pri.sh @@ -66,6 +66,12 @@ TODO: 4 added. TODO: 4 of 4 tasks shown EOF +test_todo_session 'priority error' <>> todo.sh pri 10 B +=== 1 +TODO: No task 10. +EOF + cat > todo.txt < Date: Sat, 17 Dec 2011 22:36:02 +0100 Subject: [PATCH 2/3] Refactoring: Extract getTodo() and getNewtodo() functions. The retrieval of a task text for $item and associated error handling so far was scattered around the individual actions. This is now consolidated in two new utility functions, which directly set $todo or $newtodo, respectively. (Inconsistent variable names like $NEWTODO have been adapted.) This ensures that all actions perform the same error checking, reduces a bit of duplication, and allows custom add-ons to benefit from these exported functions. Ah, and the error messages for the "move" action is now more in line with the other errors; unfortunately, this isn't yet covered by a test. Note that the check whether $item is numeric must not use the +([0-9]) extglob any more, as such functions cannot be exported; a new Bash doesn't have the "shopt -s extglob" and complains with a syntax error. Fortunately, it is possible to perform the same check via standard Bash mechanisms. --- todo.sh | 107 +++++++++++++++++++++++++++++++------------------------- 1 file changed, 59 insertions(+), 48 deletions(-) diff --git a/todo.sh b/todo.sh index 1357c20..0157883 100755 --- a/todo.sh +++ b/todo.sh @@ -296,6 +296,11 @@ cleanup() cleaninput() { + # Parameters: When $1 = "for sed", performs additional escaping for use + # in sed substitution with "|" separators. + # Precondition: $input contains text to be cleaned. + # Postcondition: Modifies $input. + # Replace CR and LF with space; tasks always comprise a single line. input=${input//$'\r'/ } input=${input//$'\n'/ } @@ -310,6 +315,35 @@ cleaninput() fi } +getTodo() +{ + # Parameters: $1: task number + # $2: Optional todo file + # Precondition: $errmsg contains usage message. + # Postcondition: $todo contains task text. + + local item=$1 + [ -z "$item" ] && die "$errmsg" + [ "${item//[0-9]/}" ] && die "$errmsg" + + todo=$(sed "$item!d" "${2:-$TODO_FILE}") + [ -z "$todo" ] && die "TODO: No task $item${2:+ in $2}." +} +getNewtodo() +{ + # Parameters: $1: task number + # $2: Optional todo file + # Precondition: None. + # Postcondition: $newtodo contains task text. + + local item=$1 + [ -z "$item" ] && die 'Programming error: $item should exist.' + [ "${item//[0-9]/}" ] && die 'Programming error: $item should be numeric.' + + newtodo=$(sed "$item!d" "${2:-$TODO_FILE}") + [ -z "$newtodo" ] && die "TODO: No updated task $item${2:+ in $2}." +} + archive() { #defragment blank lines @@ -338,12 +372,7 @@ replaceOrPrepend() ;; esac shift; item=$1; shift - - [ -z "$item" ] && die "$errmsg" - [[ "$item" = +([0-9]) ]] || die "$errmsg" - - todo=$(sed "$item!d" "$TODO_FILE") - [ -z "$todo" ] && die "TODO: No task $item." + getTodo "$item" if [[ -z "$1" && $TODOTXT_FORCE = 0 ]]; then echo -n "$querytext" @@ -368,7 +397,7 @@ replaceOrPrepend() # date again. sed -i.bak -e "$item s/^${priority}${prepdate}//" -e "$item s|^.*|${priority}${prepdate}${input}${backref}|" "$TODO_FILE" if [ $TODOTXT_VERBOSE -gt 0 ]; then - newtodo=$(sed "$item!d" "$TODO_FILE") + getNewtodo "$item" case "$action" in replace) echo "$item $todo" @@ -797,7 +826,7 @@ _list() { fi } -export -f cleaninput shellquote filtercommand _list die +export -f cleaninput getTodo getNewtodo shellquote filtercommand _list die # == HANDLE ACTION == action=$( printf "%s\n" "$ACTION" | tr 'A-Z' 'a-z' ) @@ -874,11 +903,8 @@ case $action in "append" | "app" ) errmsg="usage: $TODO_SH append ITEM# \"TEXT TO APPEND\"" shift; item=$1; shift + getTodo "$item" - [ -z "$item" ] && die "$errmsg" - [[ "$item" = +([0-9]) ]] || die "$errmsg" - todo=$(sed "$item!d" "$TODO_FILE") - [ -z "$todo" ] && die "TODO: No task $item." if [[ -z "$1" && $TODOTXT_FORCE = 0 ]]; then echo -n "Append: " read input @@ -893,7 +919,7 @@ case $action in if sed -i.bak $item" s|^.*|&${appendspace}${input}|" "$TODO_FILE"; then if [ $TODOTXT_VERBOSE -gt 0 ]; then - newtodo=$(sed "$item!d" "$TODO_FILE") + getNewtodo "$item" echo "$item $newtodo" fi else @@ -908,14 +934,11 @@ case $action in # replace deleted line with a blank line when TODOTXT_PRESERVE_LINE_NUMBERS is 1 errmsg="usage: $TODO_SH del ITEM# [TERM]" item=$2 - [ -z "$item" ] && die "$errmsg" - [[ "$item" = +([0-9]) ]] || die "$errmsg" - DELETEME=$(sed "$item!d" "$TODO_FILE") - [ -z "$DELETEME" ] && die "TODO: No task $item." + getTodo "$item" if [ -z "$3" ]; then if [ $TODOTXT_FORCE = 0 ]; then - echo "Delete '$DELETEME'? (y/n)" + echo "Delete '$todo'? (y/n)" read ANSWER else ANSWER="y" @@ -929,7 +952,7 @@ case $action in sed -i.bak -e $item"s/^.*//" "$TODO_FILE" fi if [ $TODOTXT_VERBOSE -gt 0 ]; then - echo "$item $DELETEME" + echo "$item $todo" echo "TODO: $item deleted." fi else @@ -943,13 +966,13 @@ case $action in -e $item"s/ *$3 */ /g" \ -e $item"s/$3//g" \ "$TODO_FILE" - newtodo=$(sed "$item!d" "$TODO_FILE") - if [ "$DELETEME" = "$newtodo" ]; then - [ $TODOTXT_VERBOSE -gt 0 ] && echo "$item $DELETEME" + getNewtodo "$item" + if [ "$todo" = "$newtodo" ]; then + [ $TODOTXT_VERBOSE -gt 0 ] && echo "$item $todo" die "TODO: '$3' not found; no removal done." fi if [ $TODOTXT_VERBOSE -gt 0 ]; then - echo "$item $DELETEME" + echo "$item $todo" echo "TODO: Removed '$3' from task." echo "$item $newtodo" fi @@ -964,15 +987,13 @@ case $action in # Split multiple depri's, if comma separated change to whitespace separated # Loop the 'depri' function for each item for item in $(echo $* | tr ',' ' '); do - [[ "$item" = +([0-9]) ]] || die "$errmsg" - todo=$(sed "$item!d" "$TODO_FILE") - [ -z "$todo" ] && die "TODO: No task $item." + getTodo "$item" if [[ "$todo" = \(?\)\ * ]]; then sed -i.bak -e $item"s/^(.) //" "$TODO_FILE" if [ $TODOTXT_VERBOSE -gt 0 ]; then - NEWTODO=$(sed "$item!d" "$TODO_FILE") - echo "$item $NEWTODO" + getNewtodo "$item" + echo "$item $newtodo" echo "TODO: $item deprioritized." fi else @@ -990,11 +1011,7 @@ case $action in # Split multiple do's, if comma separated change to whitespace separated # Loop the 'do' function for each item for item in $(echo $* | tr ',' ' '); do - [ -z "$item" ] && die "$errmsg" - [[ "$item" = +([0-9]) ]] || die "$errmsg" - - todo=$(sed "$item!d" "$TODO_FILE") - [ -z "$todo" ] && die "TODO: No task $item." + getTodo "$item" # Check if this item has already been done if [ "${todo:0:2}" != "x " ]; then @@ -1003,7 +1020,7 @@ case $action in sed -i.bak $item"s/^(.) //" "$TODO_FILE" sed -i.bak $item"s|^|x $now |" "$TODO_FILE" if [ $TODOTXT_VERBOSE -gt 0 ]; then - newtodo=$(sed "$item!d" "$TODO_FILE") + getNewtodo "$item" echo "$item $newtodo" echo "TODO: $item marked as done." fi @@ -1086,19 +1103,16 @@ case $action in dest="$TODO_DIR/$3" src="$TODO_DIR/$4" - [ -z "$item" ] && die "$errmsg" [ -z "$4" ] && src="$TODO_FILE" [ -z "$dest" ] && die "$errmsg" - [[ "$item" = +([0-9]) ]] || die "$errmsg" - [ -f "$src" ] || die "TODO: Source file $src does not exist." [ -f "$dest" ] || die "TODO: Destination file $dest does not exist." - MOVEME=$(sed "$item!d" "$src") - [ -z "$MOVEME" ] && die "$item: No such item in $src." + getTodo "$item" "$src" + [ -z "$todo" ] && die "$item: No such item in $src." if [ $TODOTXT_FORCE = 0 ]; then - echo "Move '$MOVEME' from $src to $dest? (y/n)" + echo "Move '$todo' from $src to $dest? (y/n)" read ANSWER else ANSWER="y" @@ -1111,10 +1125,10 @@ case $action in # leave blank line behind (preserves line numbers) sed -i.bak -e $item"s/^.*//" "$src" fi - echo "$MOVEME" >> "$dest" + echo "$todo" >> "$dest" if [ $TODOTXT_VERBOSE -gt 0 ]; then - echo "$item $MOVEME" + echo "$item $todo" echo "TODO: $item moved from '$src' to '$dest'." fi else @@ -1135,11 +1149,8 @@ case $action in note: PRIORITY must be anywhere from A to Z." [ "$#" -ne 3 ] && die "$errmsg" - [[ "$item" = +([0-9]) ]] || die "$errmsg" [[ "$newpri" = @([A-Z]) ]] || die "$errmsg" - - todo=$(sed "$item!d" "$TODO_FILE") - [ -z "$todo" ] && die "TODO: No task $item." + getTodo "$item" oldpri= if [[ "$todo" = \(?\)\ * ]]; then @@ -1150,8 +1161,8 @@ note: PRIORITY must be anywhere from A to Z." sed -i.bak -e $item"s/^(.) //" -e $item"s/^/($newpri) /" "$TODO_FILE" fi if [ $TODOTXT_VERBOSE -gt 0 ]; then - NEWTODO=$(sed "$item!d" "$TODO_FILE") - echo "$item $NEWTODO" + getNewtodo "$item" + echo "$item $newtodo" if [ "$oldpri" != "$newpri" ]; then if [ "$oldpri" ]; then echo "TODO: $item re-prioritized from ($oldpri) to ($newpri)." From 388ae745afa875feb8a22a44a9b0b2de53802d7b Mon Sep 17 00:00:00 2001 From: Ingo Karkat Date: Sun, 18 Dec 2011 21:44:47 +0100 Subject: [PATCH 3/3] Refactoring: Extract getPrefix() for more consistent move error. I think that the error on the "move dest src" action should be given like "SRC: No task 42" instead of "TODO: No task 42 in /path/to/src.txt", to be consistent with the addto and listfile actions. Extracted and exposed getPrefix(), again to remove a bit of duplication, and because this can be useful in custom add-ons, too. --- todo.sh | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/todo.sh b/todo.sh index 0157883..ee5a3f1 100755 --- a/todo.sh +++ b/todo.sh @@ -315,6 +315,15 @@ cleaninput() fi } +getPrefix() +{ + # Parameters: $1: todo file; empty means $TODO_FILE. + # Returns: Uppercase FILE prefix to be used in place of "TODO:" where + # a different todo file can be specified. + local base=$(basename "${1:-$TODO_FILE}") + echo "${base%%.[^.]*}" | tr 'a-z' 'A-Z' +} + getTodo() { # Parameters: $1: task number @@ -327,7 +336,7 @@ getTodo() [ "${item//[0-9]/}" ] && die "$errmsg" todo=$(sed "$item!d" "${2:-$TODO_FILE}") - [ -z "$todo" ] && die "TODO: No task $item${2:+ in $2}." + [ -z "$todo" ] && die "$(getPrefix "$2"): No task $item." } getNewtodo() { @@ -341,7 +350,7 @@ getNewtodo() [ "${item//[0-9]/}" ] && die 'Programming error: $item should be numeric.' newtodo=$(sed "$item!d" "${2:-$TODO_FILE}") - [ -z "$newtodo" ] && die "TODO: No updated task $item${2:+ in $2}." + [ -z "$newtodo" ] && die "$(getPrefix "$2"): No updated task $item." } archive() @@ -687,10 +696,8 @@ _addto() { echo "$input" >> "$file" if [ $TODOTXT_VERBOSE -gt 0 ]; then TASKNUM=$(sed -n '$ =' "$file") - BASE=$(basename "$file") - PREFIX=$(echo ${BASE%%.[^.]*} | tr 'a-z' 'A-Z') echo "$TASKNUM $input" - echo "${PREFIX}: $TASKNUM added." + echo "$(getPrefix "$file"): $TASKNUM added." fi } @@ -813,20 +820,18 @@ _list() { [ "$filtered_items" ] && echo "$filtered_items" if [ $TODOTXT_VERBOSE -gt 0 ]; then - BASE=$(basename "$FILE") - PREFIX=$(echo ${BASE%%.[^.]*} | tr 'a-z' 'A-Z') NUMTASKS=$( echo -n "$filtered_items" | sed -n '$ =' ) TOTALTASKS=$( echo -n "$items" | sed -n '$ =' ) echo "--" - echo "${PREFIX}: ${NUMTASKS:-0} of ${TOTALTASKS:-0} tasks shown" + echo "$(getPrefix "$FILE"): ${NUMTASKS:-0} of ${TOTALTASKS:-0} tasks shown" fi if [ $TODOTXT_VERBOSE -gt 1 ]; then echo "TODO DEBUG: Filter Command was: ${filter_command:-cat}" fi } -export -f cleaninput getTodo getNewtodo shellquote filtercommand _list die +export -f cleaninput getPrefix getTodo getNewtodo shellquote filtercommand _list die # == HANDLE ACTION == action=$( printf "%s\n" "$ACTION" | tr 'A-Z' 'a-z' )