Make Address optional #74
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
|
|
|
||
| // no address | ||
| Person expectedPersonNoAddress = new PersonBuilder(CLIVE).build(); | ||
| // address tag with blank address |
There was a problem hiding this comment.
Similar comment to email and phone PRs, could leave a line break for consistency?
| @@ -12,10 +12,10 @@ public class Address { | |||
| public static final String MESSAGE_CONSTRAINTS = "Addresses can take any values, and it should not be blank"; | |||
|
|
|||
| /* | |||
| * The first character of the address must not be a whitespace, | |||
| * otherwise " " (a blank string) becomes a valid input. | |||
| * As the address is now optional, the validation regex accepts a blank address | |||
There was a problem hiding this comment.
Maybe the comment shouldn't include "now"?
Maybe "Validation regex accepts blank address string because address is optional" would be easier to understand for future devs who have no background knowledge that this was from AB3?
| @@ -56,7 +56,7 @@ public EditCommand parse(String args) throws ParseException { | |||
| editPersonDescriptor.setEmail(ParserUtil.parseEmail(argMultimap.getValue(PREFIX_EMAIL).get())); | |||
| } | |||
| if (argMultimap.getValue(PREFIX_ADDRESS).isPresent()) { | |||
| editPersonDescriptor.setAddress(ParserUtil.parseAddress(argMultimap.getValue(PREFIX_ADDRESS).get())); | |||
| editPersonDescriptor.setAddress(ParserUtil.parseAddress(argMultimap.getValue(PREFIX_ADDRESS).orElse(""))); | |||
There was a problem hiding this comment.
I could be misunderstanding this but if argMultimap.getValue(PREFIX_ADDRESS).isPresent() is true, then editPersonDescriptor.setAddress(ParserUtil.parseAddress(argMultimap.getValue(PREFIX_ADDRESS).orElse(""))) will always return the argMultimap.getValue(PREFIX_ADDRESS) value, even if it is a blank string (ie: redundency...?).
Please correct me if I am wrong
| @@ -84,11 +91,10 @@ public void toModelType_nullEmail_throwsIllegalValueException() { | |||
| } | |||
|
|
|||
| @Test | |||
| public void toModelType_invalidAddress_throwsIllegalValueException() { | |||
| public void toModelType_blankAddress_returnsPerson() throws Exception { | |||
There was a problem hiding this comment.
Would it better to declare that this function throws IllegalValueException (from person.toModelType())? I see that there is a testcase above that just throws Exception but maybe IllegalValueException would be better?
Fixes #55