-
Notifications
You must be signed in to change notification settings - Fork 713
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 issue #361 - add persistence for priority on completion, as an option defaulting to original behavior #362
base: master
Are you sure you want to change the base?
Conversation
…s an option defaulting to original behavior
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You've just adapted the existing tests (and added manual QA steps in the PR text itself) - I'd like to have this covered (both command-line option and the config variable similar to TODOTXT_DATE_ON_ADD=1
in t1030-addto-date.sh
) to prevent any regressions in that area.
The spec recommends to use metadata (i.e. turning (A)
into pri:a
) instead of keeping the priority as-is. Would that work for you, too?
@@ -21,7 +21,7 @@ test_expect_success 'no config file' ' | |||
|
|||
# All the below tests will output the usage message. | |||
cat > expect <<EOF | |||
Usage: todo.sh [-fhpantvV] [-d todo_config] action [task_number] [task_description] | |||
Usage: todo.sh [-fhpamntvV] [-d todo_config] action [task_number] [task_description] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder whether this needs to be exposed as a command-line option - it struck me as noteworthy that most of your proposed change is about these boilerplate modifications, and the actual extension is just a few lines.
I guess most users would like to permanently configure TODOTXT_PRESERVE_PRIORITY=1
in the config file, anyway. (The same argument applies to the existing auto-archive, preserve line-numbers, and date prepending; unfortunately, these already have set the precedent in favor of short options, and it's getting increasingly more difficult to find good candidate letters.)
For my own scripts, I'd just offer a long GNU-style command-line option a la --keep-priority-on-done
for this. But this would mean switching to GNU getopt
(which is a separate dependency on Mac OS), and many adaptations. (And I wouldn't expect you to do this as part of this PR - this is just me brainstorming for the co-developer.)
@@ -113,6 +113,8 @@ $indentedJoinedConfigFileLocations | |||
Don't auto-archive tasks automatically on completion | |||
-A | |||
Auto-archive tasks automatically on completion | |||
-m | |||
Maintain priority field on completion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the "maintain" is meant to establish a mnemonic for the chosen option letter, but it sounds a bit confusing to my non-native ears. I'd prefer Don't remove a priority on completion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, on both points.
@@ -1231,7 +1242,7 @@ case $action in | |||
if [ "${todo:0:2}" != "x " ]; then | |||
now=$(date '+%Y-%m-%d') | |||
# remove priority once item is done | |||
sed -i.bak "${item}s/^(.) //" "$TODO_FILE" | |||
[[ "$TODOTXT_PRESERVE_PRIORITY" = "1" ]] || sed -i.bak $item"s/^(.) //" "$TODO_FILE" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those purely cosmetic, I'd prefer to keep the variable $item
inside the double quotes as "${item}s/^(.) //"
. My shellcheck isn't clever enough to figure out that the word-split variable contents can safely be kept outside quotes.
@wwalker Is this still work in progress? |
Before submitting a pull request, please make sure the following is done:
master
.fixes #XX
reference to the issue that this pull request fixes.Reviewer, manual QA of change:
Setup:
Testing:
Verify no change in existing functionality:
Run:
Verify that the Priority is removed, as is the currently expected behavior:
Verify that the new feature works (allow persistence of priority after completion, if user specifies
-m
):Run:
Verify that the Priority persists after the completion: