-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add Postal Code #55
Add Postal Code #55
Conversation
* 'master' of https://github.com/KhoonSun47/tp: Update EstateEase Developer Guide Add UC for home-seller requirement filtering Add UC for viewing home-buyer and home-seller requirements Add use case id Update numbering in use cases Omit the UI details in use cases Add new informations to the developer guide Remove traces of AB3 Add new use cases to the developer guide
* 'master' of https://github.com/KhoonSun47/tp: Add use cases for US5, US13, US18 Rearrange the sequence of use cases Update user story table
- Add PostalCode, PostalCodeTest: to support add postal code functionalities - Update AddCommand: update add command to add postal code * Take note that current commit is only able to add postal code, but cannot edit postal code. Editing postal code will be done at the next iterations. * Take note that adding of postal code to Person is the first version, since we will be adding buyer and seller, and then attached the postal code to them instead. - Add two new packages: 'Houses' under main and 'Houses' under test - Update test cases and json files to have postal codes
LogicManagerTest, CommandTestUtil, AddCommandParserTest, ParserUtilTest, PersonTest, JsonAdaptedPersonTest, PersonBuilder, PersonUtil, TypicalPersons.java: Fix stylecheck issues
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #55 +/- ##
============================================
+ Coverage 75.26% 75.79% +0.53%
- Complexity 419 431 +12
============================================
Files 71 72 +1
Lines 1338 1376 +38
Branches 126 131 +5
============================================
+ Hits 1007 1043 +36
- Misses 301 303 +2
Partials 30 30 ☔ View full report in Codecov by Sentry. |
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.
Other than the 1 comment I mentioned below, the rest looks good to me 👍👍
@@ -101,7 +101,8 @@ private static Person createEditedPerson(Person personToEdit, EditPersonDescript | |||
Address updatedAddress = editPersonDescriptor.getAddress().orElse(personToEdit.getAddress()); | |||
Set<Tag> updatedTags = editPersonDescriptor.getTags().orElse(personToEdit.getTags()); | |||
|
|||
return new Person(updatedName, updatedPhone, updatedEmail, updatedAddress, updatedTags); | |||
return new Person(updatedName, updatedPhone, updatedEmail, updatedAddress, | |||
personToEdit.getPostalCode(), updatedTags); |
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.
personToEdit.getPostalCode()
this would return the original postal code instead of the new value.
But isn't this edit command? 😅😅 I don't think you have to touch this.
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.
Yeap, I set it to be like this because I have not officially change the edit function yet for postal code. I think if I did not make any changes to the edit, the test cases for the edit will not pass, and hence the build will not go through at the GitHub. The current edit implementation is just temporary, I will edit this when I doing the next iteration. But yes, thanks for noticing and bringing it up :)
Update ParserUtil.java: Change one of the method to return correct result Update ParserUtilTest.java: Add one missing test case
@@ -56,7 +56,7 @@ public class EditCommand extends Command { | |||
private final EditPersonDescriptor editPersonDescriptor; | |||
|
|||
/** | |||
* @param index of the person in the filtered person list to edit | |||
* @param index of the person in the filtered person list to edit |
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.
Should the extra space be removed?
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.
LGTM. Just some syntax issue (additional space).
@@ -11,5 +11,6 @@ public class CliSyntax { | |||
public static final Prefix PREFIX_EMAIL = new Prefix("e/"); | |||
public static final Prefix PREFIX_ADDRESS = new Prefix("a/"); | |||
public static final Prefix PREFIX_TAG = new Prefix("t/"); | |||
public static final Prefix PREFIX_POSTALCODE = new Prefix("postal/"); |
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.
Perhaps using 'pc/' instead of 'postal/' would allow users to type faster?
Add PostalCode, PostalCodeTest: to support add postal code functionalities
Update AddCommand: update add command to add postal code
Editing of the postal code will need to be done during the next iterations.
Add two new packages: 'Houses' under main and 'Houses' under test.
Update test cases and json files to have postal codes.