-
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 role-related commands and checks for valid roles #102
Add role-related commands and checks for valid roles #102
Conversation
Codecov Report
@@ Coverage Diff @@
## master #102 +/- ##
============================================
- Coverage 68.03% 65.02% -3.02%
- Complexity 651 666 +15
============================================
Files 117 125 +8
Lines 2212 2410 +198
Branches 258 283 +25
============================================
+ Hits 1505 1567 +62
- Misses 629 754 +125
- Partials 78 89 +11
Continue to review full report at Codecov.
|
*/ | ||
void deleteAssignment(Assignment target); | ||
void deleteAssignment(Assignment target) throws AssignmentNotFoundException; |
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.
Small comment but I think we don't have to throw this, since AssignmentNotFoundException is a runtime exception? Also a bit inconsistent with other methods like deleteWorker/deleteShift which do not throw WorkerNotFoundException/ShiftNotFoundException respectively
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.
It seems that it is a practice to make RuntimeException
s checked exceptions if the client is able to recover from them (https://docs.oracle.com/javase/tutorial/essential/exceptions/runtime.html). But I do agree it is inconsistent with the other methods so I have changed the implementation of UnassignCommand#execute()
itself.
*/ | ||
public void remove(Assignment toRemove) { | ||
public void remove(Assignment toRemove) throws AssignmentNotFoundException { |
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.
Same small issue!
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
Resolves #81