-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add shift-add and shift-delete commands and make commands case-insensitive #42
Add shift-add and shift-delete commands and make commands case-insensitive #42
Conversation
Codecov Report
@@ Coverage Diff @@
## master #42 +/- ##
============================================
- Coverage 56.05% 54.56% -1.50%
Complexity 410 410
============================================
Files 87 91 +4
Lines 1643 1688 +45
Branches 183 189 +6
============================================
Hits 921 921
- Misses 684 729 +45
Partials 38 38
Continue to review full report at Codecov.
|
Currently, the shift-add and shift-edit commands do not support role requirements that uses spaces (e.g. Fry Cook) #44 |
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.
LGTM! Good fix on the lower case commands. Added a question, but approving as that comment isn't really about the code you are pushing.
@@ -173,8 +173,8 @@ Format: `shift edit SHIFT_INDEX [d/DAY] [t/TIME] [r/ROLE-NUMBER_NEEDED]...` | |||
* Each role should be accompanied by the number needed, and this number **must be 0 (0 will remove the role from the shift) or a positive integer** 1, 2, 3... |
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.
Just tested with the current master
branch code, 0 does not remove the role from the shift yet, but instead shift-edit removes all roles, then add the ones keyed in. Do we want to have the 0 = remove functionality, or just not allow 0 to be keyed in?
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 we'll still go with 0 removing the role
@@ -163,7 +163,7 @@ Format: `shift list` | |||
|
|||
Edits the details of an existing shift in the McScheduler. | |||
|
|||
Format: `shift edit SHIFT_INDEX [d/DAY] [t/TIME] [r/ROLE-NUMBER_NEEDED]...` | |||
Format: `shift edit SHIFT_INDEX [d/DAY] [t/TIME] [r/ROLE <SPACE> NUMBER_NEEDED]...` |
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.
Thanks for helping to update the user guide.
Resolves #10
Resolves #12
Resolves #15
Test cases will be added in future commits