-
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 Schedule Feature (Partial) #142
Add Schedule Feature (Partial) #142
Conversation
LGTM |
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 but it seems there are some merge conflicts. Also see my comment below (no change necessarily required).
|
||
/** | ||
* The API of the Model component. | ||
*/ | ||
public interface Model { | ||
public interface Model extends ScheduleModel { |
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 am slightly concerned about having Model extend ScheduleModel in terms of design, but I understand why it's done this way for abstraction and to maintain compatibility. If there is no better solution then it LGTM. Perhaps in a future PR we can abstract the different model interfaces out (for tutor, appointment, etc) and then have Model extend all of them for consistency.
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.
Noted, then I'll shift it back to Model for meantime. If we decided to adopt this approach then I'll reinstate back in this file
Fixed merge conflicts and shifting of Schedule's interface methods to |
Codecov Report
@@ Coverage Diff @@
## master #142 +/- ##
============================================
- Coverage 51.97% 48.67% -3.31%
- Complexity 621 625 +4
============================================
Files 143 158 +15
Lines 2832 3059 +227
Branches 329 359 +30
============================================
+ Hits 1472 1489 +17
- Misses 1258 1467 +209
- Partials 102 103 +1
Continue to review full report at Codecov.
|
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
LGTM |
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
Part 1 of #133
Changelog
Event
(an abstract superclass ofAppointment
andSchedule
). Purpose of this superclass to allow the new proposed Timetable Ui to show bothAppointment
andSchedule
.Appointment
to inherit fromEvent
Schedule
and its respective variable classesEventList
,ScheduleList
andScheduleTracker
AddScheduleCommand
andDeleteScheduleCommand
and its respective Parser.Upon merging this PR, the next PR will include (Completed):
ListScheduleCommand
andEditScheduleCommand
and its respective parsersIn the upcoming PR:
ModelManager
constructor to take inScheduleTracker
(not implemented in this PR to avoid huge PR)Reminder
and its respective commands