Branch v1.2 #93
Branch v1.2 #93
Conversation
execute() now throws DuplicatePersonWithoutMergePromptException if compulsory fields are equal and only throws DuplicatePersonWithMergePromptException if any compulsory field is different.
…nfirmedCommand class
…ied in user input
…to AGE_ZERO and AGE_INFINITY respectively if it was not specified in user input
…that should not be entered
There was a problem hiding this comment.
This is some amazing work man; i'm really impressed by your feature (and how much you got done.) Good job yi yin!
A few pointers from me:
-
I'd recommend to try and break your PR down into smaller chunks (ie coverage bug can be in another PR) for the advantages of adopting CI. Some em off the top of my head include 1) lesser time resolving merge conflicts 2) easier to identify which PR broke master branch 3) better PR reviews (easier for the reviewer to review small PRs than large ones) .
-
I'd recommend to update
DeveloperGuide.adocabout how to merge commands too. It might save time in the long run as you can ask the rest of the team (like me) to refer to the dev guide if they aren't sure how to use the merge command.
good stuff once agn!
| @@ -29,22 +28,19 @@ | |||
| + PREFIX_EMAIL + "EMAIL " | |||
| + PREFIX_ADDRESS + "ADDRESS " | |||
| + PREFIX_DATE_OF_BIRTH + "DATE OF BIRTH " | |||
There was a problem hiding this comment.
Missing line break after PREFIX_DATE_OF_BIRTH
| @@ -14,6 +14,7 @@ | |||
| + "NRICs should only contain alphanumeric characters. It should start with S, T, F or G" | |||
| + " followed by 6 numerical numbers and a checksum letter. It should not be blank.\n"; | |||
|
|
|||
There was a problem hiding this comment.
NRIC regex is wrong. Should be 7 digits instead of 6. Also can consider using this instead as it's stricter (must start with [STFG], end with [A-Z])
^[STFG]\d{7}[A-Z]$
https://stackoverflow.com/questions/29743154/regular-expression-for-nric-fin-in-singapore
| case HelpCommand.COMMAND_WORD: | ||
| return new HelpCommand(); | ||
|
|
||
| case MergeCommand.COMMAND_WORD: |
There was a problem hiding this comment.
Bug: Does not parse any other command if I type merge.
Steps to reproduce bug:
- Type
merge - Type any other command (ie
listpolicy)
-> Commandbox will showunknown command
| * @throws CommandException Should not be thrown. | ||
| * @throws ParseException Should not be thrown. | ||
| */ | ||
| private void handleInitialisingMergeCommand(String command) throws CommandException, ParseException { |
There was a problem hiding this comment.
Bug: When I click enter, it does not merge my command; enter should default to yes.
Solution: Flip the logic around.
if (command.equals("no")) {...} else {...}
| } | ||
| return rejectCommand; | ||
| default: | ||
| throw new ParseException(MESSAGE_UNKNOWN_COMMAND); |
There was a problem hiding this comment.
Bug: If I type another command in while merging, it says unknown command and i'm not able to proceed further. Should be the same bug I mentioned below
There was a problem hiding this comment.
Oh this one was intentional though. I don't think the user should be keying other commands in between the merge.
Add new execute and parseCommand method to take in a boolean isSystemInput to differentiate between user's invalid command "merge" and a system invoked "merge"
# Conflicts: # src/main/java/seedu/address/logic/parser/AddressBookParser.java
Add merging feature for duplicate persons.
Fixed coverage bug.
Display policies in the same way as tags for PersonListCard.