Closed Pegasust closed 2 years ago
Good catch! The duplicated priority indeed is a bug.
I think that ignoring an entered priority is the wrong solution; instead, that priority should replace an existing one, like with dates. You unfortunately did not add any new tests that show the new behavior (and instead deleted and then restored an existing test, and added other unrelated stuff in the PR - please do separate submittals in the future!)
In #387, I've reworked the replacement algorithm to do a pull merge of priority + date (and added tests to show the new behavior). How do you like that?
Regarding 1a2a8ed
(#386): The gitignore
doesn't have any editor-specific settings; I think things like ignoring .vscode
folder should go into a user-specific ~/.gitignore
instead.
Regarding e4708d9
(#386): I don't understand the motivation for removing the dummy action for test skips; why is that a problem?
Thanks for reviewing my code.
Regarding the removal of the dummy action for test skips, to put it simply, the test above supposedly produces a side-effect (remove foo
from list of action). The skipping mechanism doesn't produce this side-effect, while the test below it (not skipped) assumes this side-effect. So the test after it fails.
Regarding the editor-specific settings, I think your argument is valid.
I'll take a look at your PR. Meanwhile, may I ask how do you include the specific commit into these comments (like e4608d9 (#386))
Hi @inkarkat, my changes are only related to the interface given to the STDIN and are consistent with the behavior of the command. Hence, it is hard to test for what is given from read -i
as it effectively pre-injects into stdin.
Before making these changes, I have also considered whether to change the behavior of todo.sh replace
to also replace priority, but there seems to be a competing command to do so: todo.sh pri <num> <new-priority>
.
In all honesty, I prefer your changes in #387 a lot better than the current behavior.
Regarding the removal of the dummy action for test skips, to put it simply, the test above supposedly produces a side-effect (remove foo from list of action). The skipping mechanism doesn't produce this side-effect, while the test below it (not skipped) assumes this side-effect. So the test after it fails.
Ah, I understand the problem now. Well spotted, thank you! I used to use Cygwin, but have switched to Ubuntu a long time ago; unfortunately, we still don't have a CI build on Cygwin (and embarassingly, the MacOS builds have been broken for a long time (@karbassi - any updates here?)), so this hasn't been noticed.
I've taken your commit, added some explanation and a small refactoring on top, and separately submitted that as #388.
how do you include the specific commit into these comments (like e4608d9 (https://github.com/todotxt/todo.txt-cli/pull/386))
Oh, that's just GitHub magic. On the /Commits\ tab on this PR, you just copy the URL to a particular commit.
In all honesty, I prefer your changes in https://github.com/todotxt/todo.txt-cli/pull/387 a lot better than the current behavior.
Great, thank you! If @karbassi agrees, we'll incorporate that extension of the behavior then.
With your Cygwin test fix in a separate PR (#388), there's then nothing left to be handled here, so I'm closing this PR.
Thanks for your bug report, fix, and explanations; I hope you'll enjoy todo.sh
as much as we do!
Changes
todo.sh replace <ID>
with stdin inputt8010-listaddons.sh
Previously
The portion surrounded by
{}
is the initial input providedAfter
Before submitting a pull request, please make sure the following is done:
master
.fixes #XX
reference to the issue that this pull request fixes.