-
Notifications
You must be signed in to change notification settings - Fork 715
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
Updated add command to accept lowercase priority #230
Conversation
Is this to add an additional priority level (A-Z, a-z) or is this to change to using a-z? I'm not quite understanding the reasoning for this. |
It actually doesn't do either of those. It leaves the existing priority level (A-Z only). Some commands accept priority input as a-z and convert to uppercase so that it works (pri, listpri). The add command doesn't convert the lowercase priority to uppercase. That means the user must use the pri command after, if they notice their mistake. It seemed like a minor inconsistency and inconvenience to require that. |
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.
Good that this makes the functional behavior consistent. Code look sound.
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.
The logic for uppercasing is cumbersome, but this is hard to do without GNU extensions to sed.
In the specs, it is noted that priority choices are from A to Z, uppercase. That means there are 26 choices. This change looks good and based on the reviews, I'm merging this. |
Actually, @dstjacques could you add a test for this? @inkarkat could help as well as he knows the test system real well. |
@dstjacques If you could leave the original test as it is and add another one to handle the lowercase test, that would be better. Each test should test one thing only. Outside of that, the code looks good to merge. Will wait for the test update then will merge. |
- Updated add command to accept lowercase priority - Added testcase for add with lowercase priority
The priority command accepts lowercase priority characters and converts them to uppercase.
The list priority command also accepts lowercase priority characters.
The add command does not currently accept lowercase priority characters.
$ ./todo.sh add "(a) Task" 1 (a) Task
This change will allow the add command to accept lowercase priority characters and address the inconsistency across commands.
$ ./todo.sh add "(a) Task" 1 (A) Task