-
Notifications
You must be signed in to change notification settings - Fork 166
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
[W5.7][F10-3]Ng Ee Hooi #462
base: master
Are you sure you want to change the base?
Conversation
Well done! I like the size command, seems like a pretty useful feature! |
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.
@eehooi Well done for the size
command 👍 Remember to add JUnit tests for the new feature. This PR is rejected as it is not a clean PR (contains LO from week 4)
Please close the PR after looking at the comments below.
@@ -21,7 +21,8 @@ public CommandResult execute() { | |||
+ "\n" + ListCommand.MESSAGE_USAGE | |||
+ "\n" + ViewCommand.MESSAGE_USAGE | |||
+ "\n" + ViewAllCommand.MESSAGE_USAGE | |||
+ "\n" + HelpCommand.MESSAGE_USAGE | |||
+ "\n" + HelpCommand.MESSAGE_USAGE |
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.
Got empty space behind
import seedu.addressbook.commands.ListCommand; | ||
import seedu.addressbook.commands.ViewAllCommand; | ||
import seedu.addressbook.commands.ViewCommand; | ||
import seedu.addressbook.commands.*; |
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.
Coding standard violation here. Need to specify all classes used in this file
@@ -90,20 +88,22 @@ public void save(AddressBook addressBook) throws StorageOperationException { | |||
*/ | |||
public AddressBook load() throws StorageOperationException { | |||
|
|||
if (!Files.exists(path) || !Files.isRegularFile(path)) { | |||
if (path.getFileSystem().isReadOnly() || !Files.exists(path) || !Files.isRegularFile(path)) { |
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.
Unrelated changes here
@@ -10,6 +10,36 @@ | |||
|
|||
public class UtilsTest { | |||
|
|||
@Test | |||
public void isAnyNull() throws Exception { |
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 as above. Always remember to create a new branch off master
for each enhancement.
add new size command