Skip to content
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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

eehooi
Copy link

@eehooi eehooi commented Sep 13, 2018

add new size command

@ganchinyao
Copy link

Well done! I like the size command, seems like a pretty useful feature!

Copy link

@dalessr dalessr left a 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
Copy link

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.*;
Copy link

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)) {
Copy link

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 {
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants