-
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
Edit applicant entry #88
Edit applicant entry #88
Conversation
✅ Deploy Preview for shiny-daffodil-45f25f canceled.
|
Codecov Report
@@ Coverage Diff @@
## master #88 +/- ##
============================================
+ Coverage 73.97% 74.11% +0.13%
- Complexity 452 458 +6
============================================
Files 77 77
Lines 1483 1491 +8
Branches 139 141 +2
============================================
+ Hits 1097 1105 +8
Misses 351 351
Partials 35 35
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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 😄
Minor typo to fix, but the other test cases look great. Thanks for increasing our coverage!
@@ -78,10 +78,17 @@ public CommandResult execute(Model model) throws CommandException { | |||
Applicant applicantToEdit = lastShownList.get(index.getZeroBased()); | |||
Applicant editedApplicant = createEditedApplicant(applicantToEdit, editApplicantDescriptor); | |||
|
|||
// Check if edited applicant has the identity as another existing applicant |
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.
Did you mean "if edited applicant has the same identity as another applicant"?
requireNonNull(applicant); | ||
return applicants.numberOfDuplicates(applicant) > 1; | ||
} | ||
|
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.
This duplicate check seems necessary due to the way we are defining identity of applicants. Kudos for spotting this bug! 🎉
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
7cfd650
into
AY2324S1-CS2103T-W08-1:master
Closes #20