Add remark command #2

ZShunRen · 2024-09-28T06:12:17Z

Closed in favor of #32, due to a branch renaming from AgentAssist-RemarkCommand-Dev to tutorial-adding-command.

iamdiluxedbutcooler

Overall LGTM! Thank you so much for your valuable efforts to the tp! :D

src/main/java/seedu/address/storage/JsonAdaptedPerson.java

src/main/java/seedu/address/logic/commands/RemarkCommand.java

iamdiluxedbutcooler · 2024-09-28T14:32:43Z

src/main/java/seedu/address/logic/parser/AddCommandParser.java

-import seedu.address.model.person.Name;
-import seedu.address.model.person.Person;
-import seedu.address.model.person.Phone;
+import seedu.address.model.person.*;


How could we rewrite the stars?

Amend constructor usage in JsonAdaptedPersonTest.java file and JsonAdaptedPerson.java file. As the JsonAdaptedPerson has an additional remark parameter, the lack of this parameter in the test file's constructor calling led to an error. Thus the constructor calls have the remark parameter specified now.

iamdiluxedbutcooler · 2024-09-30T03:13:22Z

src/main/java/seedu/address/logic/commands/RemarkCommand.java

+import seedu.address.model.person.Remark;
+
+
+import java.util.List;


Not resolve yet sorry. The import java.* should be placed before other import seedu.*.

iamdiluxedbutcooler · 2024-09-30T03:14:30Z

src/main/java/seedu/address/logic/commands/RemarkCommand.java

+import seedu.address.model.person.Person;
+import seedu.address.model.person.Remark;
+
+


Eventhough having 2 blank lines is not violate the coding standard, but might ask if you wanna reduce it to one?

iamdiluxedbutcooler · 2024-09-30T03:15:48Z

src/main/java/seedu/address/model/person/Remark.java

+
+import static java.util.Objects.requireNonNull;
+import static seedu.address.commons.util.AppUtil.checkArgument;
+public class Remark {


Consider to have one blank line before declare the class! (or because we will have JavaDocs comment here anyway)

Thanks for the suggestion, will add it soon

iamdiluxedbutcooler · 2024-09-30T03:16:10Z

src/main/java/seedu/address/model/person/Remark.java

+    }
+
+    @Override
+    public int hashCode() {


nice for you to override the hashCode!

iamdiluxedbutcooler · 2024-09-30T03:21:12Z

src/test/java/seedu/address/logic/commands/RemarkCommandTest.java

+    @Test
+    public void execute_filteredList_success() {
+        showPersonAtIndex(model, INDEX_FIRST_PERSON);
+        Person firstPerson = model.getFilteredPersonList().get(INDEX_FIRST_PERSON.getZeroBased());


I noticed we have an opportunity to streamline our setup process across multiple tests. To make the test suite more maintainable and scalable, one suggestion is to create an abstract BaseTest class that includes all the common setup code in a @beforeeach method.

That's a good idea actually, we can open an issue for this if the others agree. @ZShunRen @FionaQY @colinhia

itsme-zeix

Awesome work overall! Just some nitpicks here and there that's mostly related to style.

itsme-zeix · 2024-09-28T14:30:52Z

src/main/java/seedu/address/logic/parser/RemarkCommandParser.java

+
+
+
+


Suggested change

Remove extra empty lines

itsme-zeix · 2024-09-28T14:32:31Z

src/main/java/seedu/address/model/person/Person.java

-import java.util.HashSet;
-import java.util.Objects;
-import java.util.Set;
+import java.util.*;


Suggested change

import java.util.*;

import java.util.Collections;

import java.util.HashSet;

import java.util.Objects;

import java.util.Set;

Replace wildcard import with more explicit import

itsme-zeix · 2024-09-28T14:33:08Z

src/main/java/seedu/address/model/util/SampleDataUtil.java

-import seedu.address.model.person.Name;
-import seedu.address.model.person.Person;
-import seedu.address.model.person.Phone;
+import seedu.address.model.person.*;


Suggested change

import seedu.address.model.person.*;

import seedu.address.model.person.Address;

import seedu.address.model.person.Email;

import seedu.address.model.person.Name;

import seedu.address.model.person.Person;

import seedu.address.model.person.Phone;

Replace wildcard import with more explicit import

The wildcard has been replaced, thank you!

itsme-zeix · 2024-09-28T14:34:04Z

src/main/java/seedu/address/storage/JsonAdaptedPerson.java

    }

-}
+}


Suggested change

}

}

Add EOF

itsme-zeix · 2024-10-02T11:52:03Z

src/main/java/seedu/address/storage/JsonAdaptedPerson.java

@@ -28,21 +29,24 @@ class JsonAdaptedPerson {
    private final String phone;
    private final String email;
    private final String address;
-    private final List<JsonAdaptedTag> tags = new ArrayList<>();
+    private final String remark;
+    private final List<JsonAdaptedTag> tagged = new ArrayList<>();


Personally I would retain tags instead of renaming the variable to tagged since we are referring to a list of tags. tagged doesn't quite imply list of tags - I would infer it to be a boolean variable (although isTag better implies boolean i know)

Hmm, I agree with your reasoning. I changed it because I thought the past sense made more sense, but I shall revert this change, thanks!

itsme-zeix · 2024-10-02T11:55:29Z

src/test/java/seedu/address/logic/commands/RemarkCommandTest.java

+    public void execute_deleteRemarkUnfilteredList_success() {
+        Person firstPerson = model.getFilteredPersonList().get(INDEX_FIRST_PERSON.getZeroBased());
+        Person editedPerson = new PersonBuilder(firstPerson).withRemark("").build();
+        RemarkCommand remarkCommand = new RemarkCommand(INDEX_FIRST_PERSON,
+                new Remark(editedPerson.getRemark().toString()));
+        String expectedMessage = String.format(RemarkCommand.MESSAGE_DELETE_REMARK_SUCCESS,
+                Messages.format(editedPerson));
+        Model expectedModel = new ModelManager(new AddressBook(model.getAddressBook()), new UserPrefs());
+        expectedModel.setPerson(firstPerson, editedPerson);
+        assertCommandSuccess(remarkCommand, model, expectedMessage, expectedModel);


Suggested change

public void execute_deleteRemarkUnfilteredList_success() {

Person firstPerson = model.getFilteredPersonList().get(INDEX_FIRST_PERSON.getZeroBased());

Person editedPerson = new PersonBuilder(firstPerson).withRemark("").build();

RemarkCommand remarkCommand = new RemarkCommand(INDEX_FIRST_PERSON,

new Remark(editedPerson.getRemark().toString()));

String expectedMessage = String.format(RemarkCommand.MESSAGE_DELETE_REMARK_SUCCESS,

Messages.format(editedPerson));

Model expectedModel = new ModelManager(new AddressBook(model.getAddressBook()), new UserPrefs());

expectedModel.setPerson(firstPerson, editedPerson);

assertCommandSuccess(remarkCommand, model, expectedMessage, expectedModel);

public void execute_deleteRemarkUnfilteredList_success() {

Person firstPerson = model.getFilteredPersonList().get(INDEX_FIRST_PERSON.getZeroBased());

Person editedPerson = new PersonBuilder(firstPerson).withRemark("").build();

RemarkCommand remarkCommand = new RemarkCommand(INDEX_FIRST_PERSON,

new Remark(editedPerson.getRemark().toString()));

String expectedMessage = String.format(RemarkCommand.MESSAGE_DELETE_REMARK_SUCCESS,

Messages.format(editedPerson));

Model expectedModel = new ModelManager(new AddressBook(model.getAddressBook()), new UserPrefs());

expectedModel.setPerson(firstPerson, editedPerson);

assertCommandSuccess(remarkCommand, model, expectedMessage, expectedModel);

Add new lines for better readability

itsme-zeix · 2024-10-02T11:56:58Z

src/test/java/seedu/address/logic/commands/RemarkCommandTest.java

+    @Test
+    public void execute_filteredList_success() {
+        showPersonAtIndex(model, INDEX_FIRST_PERSON);
+        Person firstPerson = model.getFilteredPersonList().get(INDEX_FIRST_PERSON.getZeroBased());


That's a good idea actually, we can open an issue for this if the others agree. @ZShunRen @FionaQY @colinhia

…hunRen/tp into AgentAssist-RemarkCommand-Dev

ZShunRen added 2 commits September 28, 2024 14:11

Add remark command

17f2536

Remove * import in EditCommand.java

8d22593

iamdiluxedbutcooler approved these changes Sep 28, 2024

View reviewed changes

src/main/java/seedu/address/storage/JsonAdaptedPerson.java Outdated Show resolved Hide resolved

src/main/java/seedu/address/logic/commands/RemarkCommand.java Outdated Show resolved Hide resolved

iamdiluxedbutcooler self-requested a review September 28, 2024 14:02

iamdiluxedbutcooler reviewed Sep 28, 2024

View reviewed changes

ZShunRen added 5 commits September 28, 2024 23:38

Fix checkstyle issues

5ef81b9

Fix checkstyle issues in test files

9bd834d

Add message format in remark test file

4f21f3f

Adjust test case files to addition of remark

206dc1d

ZShunRen force-pushed the AgentAssist-RemarkCommand-Dev branch from ee1a039 to 206dc1d Compare September 29, 2024 17:33

iamdiluxedbutcooler reviewed Sep 30, 2024

View reviewed changes

Add spacing between lines

3bdc276

itsme-zeix approved these changes Oct 2, 2024

View reviewed changes

ZShunRen added 3 commits October 2, 2024 23:09

Remove excess whitespace in remark parser

3ccde45

Add whitespace between lines in remark test

be4c367

Merge branch 'AgentAssist-RemarkCommand-Dev' of https://github.com/ZS…

cecee49

…hunRen/tp into AgentAssist-RemarkCommand-Dev

ZShunRen closed this Oct 3, 2024

ZShunRen deleted the AgentAssist-RemarkCommand-Dev branch October 3, 2024 15:17

ZShunRen mentioned this pull request Oct 3, 2024

Add remark command #32

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add remark command #2

Add remark command #2

ZShunRen commented Sep 28, 2024 •

edited

Loading

iamdiluxedbutcooler left a comment

iamdiluxedbutcooler Sep 28, 2024

iamdiluxedbutcooler Sep 30, 2024

iamdiluxedbutcooler Sep 30, 2024

iamdiluxedbutcooler Sep 30, 2024

ZShunRen Sep 30, 2024

iamdiluxedbutcooler Sep 30, 2024

iamdiluxedbutcooler Sep 30, 2024

itsme-zeix Oct 2, 2024

itsme-zeix left a comment

itsme-zeix Sep 28, 2024

itsme-zeix Sep 28, 2024

itsme-zeix Sep 28, 2024

ZShunRen Oct 2, 2024

itsme-zeix Sep 28, 2024

itsme-zeix Oct 2, 2024

ZShunRen Oct 2, 2024

itsme-zeix Oct 2, 2024

itsme-zeix Oct 2, 2024

		import seedu.address.model.person.Remark;


		import java.util.List;

		import seedu.address.model.person.Person;
		import seedu.address.model.person.Remark;

                   }
-              }
+              }

Add remark command #2

Add remark command #2

Conversation

ZShunRen commented Sep 28, 2024 • edited Loading

iamdiluxedbutcooler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itsme-zeix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZShunRen commented Sep 28, 2024 •

edited

Loading