-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Conversation
@yamgent, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Zhiyuan-Amos, @edmundmok and @eugenepeh to be potential reviewers. |
b1fd577
to
e94cf07
Compare
v1@yamgent submitted v1 for review.
Checkout this PR version locallygit fetch https://github.com/se-edu/addressbook-level4.git refs/pr/599/1/head:BRANCHNAME where |
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.
[1/9]
@Test | ||
public void parseCommand_remark() throws Exception { | ||
RemarkCommand command = (RemarkCommand) parser.parseCommand(RemarkCommand.COMMAND_WORD); | ||
assertTrue(command instanceof RemarkCommand); |
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.
if you have forced cast it in the previous line, then this must definitely be an instance of RemarkCommand
right? :P
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.
[2/9]
|
||
/** | ||
* @param index of the person in the filtered person list to edit the remark | ||
* @param remark of the person |
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.
can be clearer: remark of the person to be updated to
?
public class RemarkCommandParser implements Parser<RemarkCommand> { | ||
/** | ||
* Parses the given {@code String} of arguments in the context of the RemarkCommand | ||
* and returns an RemarkCommand object for execution. |
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.
Should we have {@code } to wrap around RemarkCommand
?
// different index -> returns false | ||
assertFalse(standardCommand.equals(new RemarkCommand(INDEX_SECOND_PERSON, VALID_REMARK_AMY))); | ||
|
||
// different remarks -> returns false |
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.
remarks
} | ||
|
||
/** | ||
* Returns an {@code RemarkCommand}. | ||
* Returns an {@code RemarkCommand} with parameters {@code index} and {@code remark} |
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.
missing period at the end of the sentence :P
assertTrue(command instanceof RemarkCommand); | ||
final String remark = "Some remark."; | ||
RemarkCommand command = (RemarkCommand) parser.parseCommand(RemarkCommand.COMMAND_WORD + " " | ||
+ INDEX_FIRST_PERSON.getOneBased() + " " + PREFIX_REMARK + " " + remark); |
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.
Should there be a space between PREFIX_REMARK
and remark
?
private RemarkCommandParser parser = new RemarkCommandParser(); | ||
|
||
@Test | ||
public void parse_indexSpecified_failure() throws Exception { |
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.
failure? :O the test code below says assertParseSuccess
XD
|
||
// have remarks | ||
Index targetIndex = INDEX_FIRST_PERSON; | ||
String userInput = targetIndex.getOneBased() + " " + PREFIX_REMARK.toString() + " " + remark; |
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.
Is there a need for " " between PREFIX_REMARK
and remark
?
public void parse_indexSpecified_failure() throws Exception { | ||
final String remark = "Some remark."; | ||
|
||
// have remarks |
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.
should be singular: remark
RemarkCommand expectedCommand = new RemarkCommand(INDEX_FIRST_PERSON, remark); | ||
assertParseSuccess(parser, userInput, expectedCommand); | ||
|
||
// no remarks |
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.
Should be singular: remark
public void parse_noFieldSpecified_failure() throws Exception { | ||
String expectedMessage = String.format(MESSAGE_INVALID_COMMAND_FORMAT, RemarkCommand.MESSAGE_USAGE); | ||
|
||
// nothing at all |
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 comment seems unnecessary because the test name already mentions that there's no field specified.
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.
Though I wonder if we should have a test where the remark
is specified but not the index.
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.
[3/9] commit message:
Modify PersonCardHandle so that future tests can take read the
remarks.
take read the remarks
:O
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.
[4/9]
@@ -36,10 +37,10 @@ public void execute() throws Exception { | |||
|
|||
@Test | |||
public void equals() { | |||
final RemarkCommand standardCommand = new RemarkCommand(INDEX_FIRST_PERSON, VALID_REMARK_AMY); | |||
final RemarkCommand standardCommand = new RemarkCommand(INDEX_FIRST_PERSON, new Remark(VALID_REMARK_AMY)); |
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.
how about let's change VALID_REMARK_AMY
to type Remark
instead?
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.
Was just following the style of this though, maybe need to put constant somewhere else?
// null -> returns false | ||
assertFalse(remark.equals(null)); | ||
|
||
// different person -> returns false |
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.
different person.... you mean different remark XD
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.
[5/9]
@@ -20,22 +21,22 @@ | |||
public static Person[] getSamplePersons() { | |||
return new Person[] { | |||
new Person(new Name("Alex Yeoh"), new Phone("87438807"), new Email("[email protected]"), | |||
new Address("Blk 30 Geylang Street 29, #06-40"), | |||
new Address("Blk 30 Geylang Street 29, #06-40"), new Remark(""), |
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.
Should we extract these out as EMPTY_REMARK
?
@@ -5,53 +5,62 @@ | |||
<phone isPrivate="false">9482424</phone> | |||
<email isPrivate="false">[email protected]</email> | |||
<address isPrivate="false">4th street</address> | |||
<remark isPrivate="false"></remark> |
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.
out of scope of this PR, but why do we have a isPrivate
in our xml file XD
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.
Seems like an old artifact that was inherited from the lower levels of addressbook
(which does have private fields). I guess we can remove it (unless anyone wants to come up with some useful features for it :P).
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.
We can create an issue and write:
"It takes approx 2 weeks of full time work to implement a new feature. Start at your own risk!" XD
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.
[8/9]:
. Just modify PersonCard's constructor to add the new method.
i don't see the new method though :O
Also, concerning commit organisation, I'm wondering if we should squash this commit with [v1 3/9] Ui: Add a placeholder for remark in PersonCard GUI
i.e. both of these commits will become 1 commit at [7/8]. This is because there's no need for us to update the Ui component at [3/9] (I think later commits are not dependent on it).
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.
[9/9]
* {@code personToEdit}. | ||
*/ | ||
private String generateSuccessMessage(Person personToEdit) { | ||
if (!remark.value.isEmpty()) { |
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.
I think we can ternary this:
String message = !remark.value.isEmpty() ? MESSAGE_ADD_REMARK_SUCCESS : MESSAGE_DELETE_REMARK_SUCCESS;
return String.format(message, personToEdit);
} | ||
|
||
/** | ||
* Generate a command execution success message based on whether the remark is added to or removed from |
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.
Generates
@@ -55,7 +122,7 @@ public void equals() { | |||
// different index -> returns false | |||
assertFalse(standardCommand.equals(new RemarkCommand(INDEX_SECOND_PERSON, new Remark(VALID_REMARK_AMY)))); | |||
|
|||
// different remarks -> returns false | |||
// different descriptor -> returns false |
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.
:O descriptor??
@@ -28,11 +33,73 @@ | |||
private Model model = new ModelManager(getTypicalAddressBook(), new UserPrefs()); | |||
|
|||
@Test | |||
public void execute() throws Exception { | |||
final String remark = "Some remark"; | |||
public void execute_addRemark_success() throws Exception { |
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.
we should specify unfiltered list i.e. addRemarkUnfilteredList
because we have test cases for filteredList
. Same for below.
public void execute() throws Exception { | ||
final String remark = "Some remark"; | ||
public void execute_addRemark_success() throws Exception { | ||
Person editedPerson = new PersonBuilder(model.getFilteredPersonList().get(INDEX_FIRST_PERSON.getZeroBased())) |
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.
Maybe we can assign the first person of the filtered list as a variable, so that in line 45, we don't have to call model.getFilteredPersonList().get(0)
. Same for tests below.
I think the "strange" commit organization - having [v1 3/9] separate from [v1 8/9] - was to "prevent student fatigue" (i.e. if we do the merge as you proposed, then the student is not going to see any visible changes in the frontend for the first 6 commits, which might discourage them). Hence the [v1 3/9] is just placed there for them to visualize how it is going to look like eventually, which hopefully will "excite" them to continue the work. Also we try to touch different areas of the application as early as possible, rather than strictly leaving the UI components to the last. |
7491051
to
c317ab9
Compare
v2@yamgent submitted v2 for review.
(📚 Archive) (📈 Interdiff between v1 and v2) Checkout this PR version locallygit fetch https://github.com/se-edu/addressbook-level4.git refs/pr/599/2/head:BRANCHNAME where |
There were a lot of PRs merged to master, and a substantial amount of architecture changes and code style changes. Apologies in advance if I didn't capture everything for the changelog below (the interdiff shall be the authoritative source):
|
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.
Interdiff looks good.
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.
Some comments on stuff I just noticed along the way
public class Remark { | ||
|
||
public static final String MESSAGE_REMARK_CONSTRAINTS = | ||
"Person remarks can take any values, can even be blank"; |
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.
No point adding this if it is not used.
public void parse_indexSpecified_success() { | ||
// have remark | ||
Index targetIndex = INDEX_FIRST_PERSON; | ||
String userInput = targetIndex.getOneBased() + " " + PREFIX_REMARK.toString() + nonEmptyRemark; |
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.
No need for the toString
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.
[2/9]
*/ | ||
public RemarkCommand(Index index, String remark) { | ||
requireNonNull(index); | ||
requireNonNull(remark); |
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.
e747390#diff-870783b7261d1855391681c89164b118R55
We can use requireAllNonNull
instead? :)
try { | ||
index = ParserUtil.parseIndex(argMultimap.getPreamble()); | ||
} catch (IllegalValueException ive) { | ||
throw new ParseException(String.format(MESSAGE_INVALID_COMMAND_FORMAT, RemarkCommand.MESSAGE_USAGE)); |
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.
Following 69728c1, we should use chained exceptions.
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.
[9/9]
|
||
private final Index index; | ||
private final Remark remark; | ||
|
||
private Person personToEdit; | ||
private Person editedPerson; |
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.
I think there's no need for these fields now.
} catch (DuplicatePersonException dpe) { | ||
throw new AssertionError("Changing target person's remark should not result in a duplicate"); | ||
} catch (PersonNotFoundException pnfe) { | ||
throw new AssertionError("The target person cannot be missing"); |
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.
Just to check, we're catching RuntimeException
s that should not be happening? Also, should we use chained exceptions here too?
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.
Yeah I keep forgetting about these, will remove it :S.
. Modify RemarkCommand to take in an Index and String for remark, and print those two parameters as the error message . Modify RemarkCommandTest to test the equals method . Add RemarkCommandParser that will know how to parse two arguments, one index and one with prefix 'r/' . Add RemarkCommandParserTest that will test different boundary values for RemarkCommandParser. . Modify AddressBookParser to use the newly implemented RemarkCommandParser . Modify AddressBookParserTest to ensure that what the user input generated the correct command.
. Add label with any random text inside PersonListCard.fxml . Add FXML annotation in PersonCard to tie the variable to the actual label. . Modify PersonCardHandle so that future tests can read the remarks in the cards.
. Add Remark to model component (copy from Address, remove the regex and rename accordingly). . Add test for Remark (to test the equals method). . Modify RemarkCommand to now take in a Remark instead of a String.
. Add `Person#getRemark()` . You may assume that the user will not be able to use the `add` and `edit` commands to modify the remarks field (i.e. the person will be created without a remark). . Modify `SampleDataUtil` to add remarks for the sample data. I recommend deleting your addressBook.xml after this step so that you can see the remarks field come to life in your application.
. Add a new method `withRemark()` for `PersonBuilder`. This method will create a new `Remark` for the person that it is currently building. . Try and use the method on any sample `Person` in `TypicalPersons`.
. Just modify PersonCard's constructor to add the new method. . For test, modify GuiTestAssert#assertCardDisplaysPerson(...) to compare the now-functioning remark label.
. Replace the error message with the actual logic to modify the remarks of a person. . Update `RemarkCommandTest` to test that the `execute()` logic works.
b0e6488
to
8e446e8
Compare
v7@yamgent submitted v7 for review.
(📚 Archive) (📈 Interdiff between v6 and v7) (📈 Range-Diff between v6 and v7) Checkout this PR version locallygit fetch https://github.com/se-edu/addressbook-level4.git refs/pr/599/7/head:BRANCHNAME where |
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.
Changes requested were made :)
Thanks for all the reviews! @Zhiyuan-Amos @pyokagan |
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.
Sorry, missed this earlier
Part of #784.
Proposed merge commit message:
Note: For demo purpose, don't merge this to master!