Skip to content
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

Update delete contact functionality #32

Merged

Conversation

CJ-Lee01
Copy link

@CJ-Lee01 CJ-Lee01 commented Oct 10, 2023

closes #13

@CJ-Lee01 CJ-Lee01 requested review from a team, wxwern and wamps-jp and removed request for a team October 10, 2023 09:49
@wxwern wxwern marked this pull request as draft October 10, 2023 09:50
@CJ-Lee01 CJ-Lee01 added this to the v1.2 milestone Oct 11, 2023
@wxwern wxwern added type.Enhancement Enhancement to an existing feature or user story (CS2103T label) priority.High Must do tasks (CS2103T label) labels Oct 16, 2023
@CJ-Lee01 CJ-Lee01 marked this pull request as draft October 17, 2023 05:21
@wxwern wxwern changed the title Branch delete contacts Update delete contact functionality Oct 17, 2023
@CJ-Lee01 CJ-Lee01 marked this pull request as ready for review October 17, 2023 14:17
Copy link

@wamps-jp wamps-jp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems okay. I think the usage of Object selector might be a bit questionable (as in it might create complications later) but for now it's fine. Just noting that for future edits, if ever.

@CJ-Lee01
Copy link
Author

CJ-Lee01 commented Oct 17, 2023

I agree that the selector being of type Object is somewhat questionable and should be somewhat changed in the future.

I wanted the selector to have either type Index or type Id but Java doesn't allow a variable to have multiple types.

I wonder if there is a way to strictly enforce that the selector is only used for the equals method, was thinking if I should wrap it in Predicate<Object> or Predicate<?>.

@CJ-Lee01 CJ-Lee01 enabled auto-merge October 17, 2023 15:37
@CJ-Lee01 CJ-Lee01 merged commit 8ca3b9a into AY2324S1-CS2103T-W08-3:master Oct 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority.High Must do tasks (CS2103T label) type.Enhancement Enhancement to an existing feature or user story (CS2103T label)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

As a user, I can delete contacts
3 participants