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

docs: update UG and PPP #210

Merged
merged 7 commits into from Nov 10, 2019

Conversation

Wingedevil
Copy link

No description provided.

@Wingedevil Wingedevil added type.Bug A bug. documentation Improvements or additions to documentation labels Nov 10, 2019
@Wingedevil Wingedevil requested a review from SQwQ November 10, 2019 10:15
@Wingedevil Wingedevil self-assigned this Nov 10, 2019
@SQwQ
Copy link

SQwQ commented Nov 10, 2019

Please resolve branch conflicts

Copy link

@Q-gabe Q-gabe left a comment

Choose a reason for hiding this comment

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

Some comments on improving the code structure but LGTM for v1.4. 👍

String aliasUsage2 = "TEST ADD n/John Smith p/91234567 e/john@smith.co.uk "
+ "a/12 Downing St Westminster, London SW1A 2AD, UK";
String aliasUsageExpected2 = "add n/John Smith p/91234567 e/john@smith.co.uk "
+ "a/12 Downing St Westminster, London SW1A 2AD, UK";

Copy link

Choose a reason for hiding this comment

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

Test Aliases could perhaps be set as a static variable in a separate TestUtil class actually. Something like TypicalAliases() could be useful for future extension of the alias feature.


expectedModel.removeAlias(alias2.toLowerCase());
assertCommandSuccess(new UnaliasCommand(alias2), model,
String.format(UnaliasCommand.MESSAGE_SUCCESS, alias2.toLowerCase()), expectedModel);
}
Copy link

Choose a reason for hiding this comment

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

Comments for each equivalence partition would be a nice addition for each individual assertCommandSuccess() call.

@@ -54,6 +58,7 @@ public void setup(String aliases) {
* </ul>
*/
public void show() {
logger.fine("Showing list of aliases.");
Copy link

Choose a reason for hiding this comment

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

Use of logger is nice!

Copy link

@SQwQ SQwQ left a comment

Choose a reason for hiding this comment

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

Doesn't pass travis atm. Otherwise LGTM

@SQwQ SQwQ merged commit bb48d01 into AY1920S1-CS2103T-F12-2:master Nov 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation type.Bug A bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants