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-2]Lim Fong Yuan #454

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

[W5.7][F10-2]Lim Fong Yuan #454

wants to merge 9 commits into from

Conversation

OrangeJuice7
Copy link

Extract a Command.getMessageUsage() method and format the help command.

@wn
Copy link

wn commented Sep 13, 2018

You are failing the unit tests.

@@ -29,10 +29,10 @@
|| Example: viewall 1
|| help: Shows program usage instructions.
|| Parameters: none
|| Example: help
|| Example: help
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the script uses a 'diff', whitespace might be causing the CI to fail.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's strange, because both my expected and my actual output do not have any whitespace at the end of line 32.

I've made a clean clone of this branch to my com and ran runtests.bat, and it reports no difference as well.

I did change the formatting of the text files while I was working on this while I was troubleshooting why the identical-looking files are being reported as different. Maybe that's the issue instead?

+ "\n" + ViewAllCommand.MESSAGE_USAGE
+ "\n" + HelpCommand.MESSAGE_USAGE
+ "\n" + ExitCommand.MESSAGE_USAGE
+ DeleteCommand.MESSAGE_USAGE
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove the new line? If you have added the "\n" in another commit, you should squash the commit to be together with this. It is unclear why you are removing new lines.

@Kelly9373
Copy link

Good job, but maybe could add more JUnit and IO test cases :)

Copy link

@jlks96 jlks96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job for attempting the LO. As mentioned by Wei Neng, you have to be mindful of trailing whitespaces. However, there will be a static code analysis tool (Checkstyle) that you will use for your AB4 to check coding style compliance.

* @return the MESSAGE_USAGE for a command.
*/
protected static String getMessageUsage(
String commandWord,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation for wrapped lines should be 8 spaces

@azhikai
Copy link

azhikai commented Sep 14, 2018

Hi Fong Yuan,

Commit titles should be less than 72 characters and have no full stops, which is why some of your commits got wrapped to the next line.

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.

6 participants