-
Notifications
You must be signed in to change notification settings - Fork 4
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 editing of Birthdays using the edit command #28
Add editing of Birthdays using the edit command #28
Conversation
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.
Just some small comments, no big issue
return true; | ||
} | ||
test = test.strip(); |
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 can consider using strip() or trim() in parseBirthday from ParserUtil although this works as well
@@ -64,7 +64,8 @@ public void toStringMethod() { | |||
+ editPersonDescriptor.getName().orElse(null) + ", phone=" | |||
+ editPersonDescriptor.getPhone().orElse(null) + ", email=" | |||
+ editPersonDescriptor.getEmail().orElse(null) + ", address=" | |||
+ editPersonDescriptor.getAddress().orElse(null) + ", tags=" | |||
+ editPersonDescriptor.getAddress().orElse(null) + ", birthday=" | |||
+ editPersonDescriptor.getBirthday().orElse(null) + ", tags=" | |||
+ editPersonDescriptor.getTags().orElse(null) + "}"; |
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 we standardize on whether to put birthday or tags in front or it doesnt really matter?
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.
Previously I encountered some bug, and I thought tags have to be at the back because tags can have multiple values so I put birthday before tags, but turns out it doesn't matter. So let's standardize and have new attributes be at the back.
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!
Implements #27 and adds more test cases for BirthdayTest