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.11r][F11-B1]Sheikh Umar #755

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

[W5.11r][F11-B1]Sheikh Umar #755

wants to merge 35 commits into from

Conversation

shumarb
Copy link

@shumarb shumarb commented Mar 4, 2018

Enhancement 1: Added a sort command.
Enhancement 2: Added an edit command.

Sheikh-Umar added 4 commits March 5, 2018 21:44
* commit 'ecf7c7ce71dd9c404c0bef9e6e872a202b35c8d9': (26 commits)
  no message
  Converts name to lowercase.
  no message
  no message
  Revert "I updated the add command to make the name entered to lowercase."
  I updated the add command to make the name entered to lowercase.
  I updated the StorageFileTest.java.
  no message
  Added a test case to StorageFileTest.java
  Revert "I updated the find command of the User Guide."
  I updated the find command of the User Guide.
  no message
  I stated the sources that I referred to for these particular java files.
  I made changes to the EditCommand and Parser java files based on feedback from Travis.
  I updated the code based on feedback from Travis.
  I updated the EditCommand, HelpCommand, Messages, and Parser java files.
  I updated the user command for the edit command.
  I corrected a typo error.
  Updated UniquePersonList based on feedback from Travis.
  I updated the Sort Command message based on feedback from Travis.
  ...
@karrui
Copy link

karrui commented Mar 8, 2018

Hey @Sheikh-Umar! It's ok, you only need to do 1 enhancement for 5.11r to get all the missing credit so far! Don't have to commit anymore, haha. I will review after tomorrow's tutorial.

Please make proper commit headers though. Use the imperative tone (You don't have to say "I did ________"), for example the commit header should just be Update user guide. Keep your commit title length less than 72 characters to prevent cutoff. Avoid committing with empty commit headers.

@shumarb
Copy link
Author

shumarb commented Mar 8, 2018

@karrui May I know if I can still get the missing credit that I missed out even if the Travis CI failed a check? I may have 2 enhancements but I made an enhancement that caused a failed test case and I'm now finding the bug.

@karrui
Copy link

karrui commented Mar 8, 2018 via email

Copy link

@karrui karrui left a comment

Choose a reason for hiding this comment

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

Comments added. Please read through comments and mention me if you have any queries!

* by updating his/her atttribute with the new value.
*/

//Solution below adapted from https://github.com/nus-cs2103-AY1718S2/addressbook-level3/pull/749/files
Copy link

Choose a reason for hiding this comment

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

We do not condone wholesale copying of enhancements for LO submissions. Do not do this again. Since you have the sort command, I will be only reviewing the sort command enhancement.

public void execute_sort_showsAllPerson() throws Exception {
// prepare expectations
TestDataHelper helper = new TestDataHelper();
Person person1 = helper.generatePersonWithName("John Tan");
Copy link

Choose a reason for hiding this comment

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

Might want to name these variables John, Ali, etc instead of person1 person2 so as to make the generation of expectedList and unorderedList more readable.

List<Person> expectedList = helper.generatePersonList(person2, person4, person3, person1);
List<Person> unorderedList = helper.generatePersonList(person1, person2, person3, person4);

AddressBook expectedAB = helper.generateAddressBook(unorderedList);
Copy link

Choose a reason for hiding this comment

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

Why is your expectedAB generated from the unorderedList? Should it not be from expectedList?
You might have meant unorderedAB instead?

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