-
Notifications
You must be signed in to change notification settings - Fork 3
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
fix tests in TaskListPanel and ToDoListEevntCard #78
fix tests in TaskListPanel and ToDoListEevntCard #78
Conversation
return "M"; | ||
} else { | ||
return "L"; | ||
} |
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.
For this bug, I think we should probably refactor the Priority class instead.
I feel that it is more "correct" for the 'value' in the Priority class to be stored as "High", "Medium" or "Low" depending on the user input. This would probably cause a few other tests to fail also, but we need to properly refactor the code by week 13, so there's no other choice.
Ideally, we should use an enum to represent priority, but that would take even more work to modify everything.
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.
This one can change if have time when everything is done
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 don't think enum (vig's suggestion) is necessary since we only have 3 possibilities.
two test cases in ShowDescriptionCommandTest cannot pass
Pull Request Test Coverage Report for Build 264
💛 - Coveralls |
*/ | ||
public class ShowDescriptionCommand extends Command { | ||
|
||
public static final String COMMAND_WORD = "show description"; |
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 feel like something like "show todo" would be a bit simpler, would also make it clear that it is a todo command.
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.
ok
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.
looks good
i guess we'll refactor the Priority thing once everything is finalized
No description provided.