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

Add remind command #16

Merged

Conversation

ChooJiaXin
Copy link
Collaborator

closes #5
Ui for reminders not yet implemented.

@ChooJiaXin ChooJiaXin self-assigned this Sep 29, 2020
@ChooJiaXin ChooJiaXin modified the milestones: v1.1, v1.2 Sep 29, 2020
@hyngkng hyngkng closed this Oct 5, 2020
@ChooJiaXin ChooJiaXin reopened this Oct 5, 2020
Copy link
Collaborator

@itsjerryho itsjerryho left a comment

Choose a reason for hiding this comment

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

Looks good to me! I just have a question

@@ -98,8 +99,9 @@ private static Person createEditedPerson(Person personToEdit, EditPersonDescript
Email updatedEmail = editPersonDescriptor.getEmail().orElse(personToEdit.getEmail());
Address updatedAddress = editPersonDescriptor.getAddress().orElse(personToEdit.getAddress());
Set<Tag> updatedTags = editPersonDescriptor.getTags().orElse(personToEdit.getTags());
Remind updatedRemind = personToEdit.getRemind();
Copy link
Collaborator

Choose a reason for hiding this comment

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

just wondering, do we need an editPersonDescriptor at this stage or later?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since we are probably going to be adding an edit feature in the future we can just leave it in first or add it to v1.2 since the implementation is all done for us already :D

Copy link
Collaborator

@hyngkng hyngkng left a comment

Choose a reason for hiding this comment

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

LGTM

@ChooJiaXin ChooJiaXin modified the milestones: v1.2, v1.2b Oct 6, 2020
@ChooJiaXin ChooJiaXin added the enhancement New feature or request label Oct 6, 2020
Copy link

@andreatanky andreatanky left a comment

Choose a reason for hiding this comment

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

LGTM!


Assignment assignment = new Assignment(name, deadline, moduleCode);

Assignment assignment = new Assignment(name, deadline, moduleCode, remind);

Choose a reason for hiding this comment

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

is there an extra line at the top?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What do you mean?

Copy link
Collaborator

@nevirmc nevirmc left a comment

Choose a reason for hiding this comment

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

I think it is good to merge. I will add more test cases later.

@ChooJiaXin ChooJiaXin merged commit 817c0af into AY2021S1-CS2103T-F11-3:master Oct 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add reminders
5 participants