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

Rebased and updated tutorial add remark #225

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

drustanyjt
Copy link

@drustanyjt drustanyjt commented Oct 30, 2024

PR to address #223.

TLDR: The purpose of this PR is to fix the outdated tutorial-add-remark branch which is used in AB3's Add Remark tutorial. 4 out of 9 of the original commits made use of methods that no longer existed, or had broken import statements due to changing project structures.

Introduction

This PR rebases commits from the tutorial-add-remark branch, and where necessary updates the code within the individual commit to ensure compatibility with modern AB3 (as of October 2024). By default git's rebase incorporates codes from the base commit (the master branch in this case) that our commits do not modify. So we are left with ensuring that the modified code for the Remark command makes use of files in the base commit properly.

Changes

The 9 rebased commit's start from 01190de, and look something like this:
image
For each of our 9 original commits, it might be easier to refer to this diff to understand the code change. Changes made manually (if any) are also briefly listed below per commit.

  1. Logic: Implement minimal remark command original commit: nil
  2. Logic: Teach the app to accept 'remark' but do nothing original commit: nil
  3. Logic: Teach the app to accept 'remark' arguments but still do nothing original commit: fix import statements
  4. Model: Add Remark class original commit: nil
  5. Ui: Add a placeholder for remark in PersonCard GUI original commit: nil
  6. Model: Modify Person to support a Remark field original commit: fix use of ToStringBuilder and missing test case
  7. Storage: Add Remark field to JsonAdaptedPerson class original commit: fix json representation of tags
  8. Ui: Connect Remark field to PersonCard original commit: nil
  9. Logic: Implement RemarkCommand execute() logic original commit: fix import statements

Verification

To check that the changes work, for each of these commits, I used Gradle's test configuration to see if all test cases passed, and run configuration to see if AB3 could compile and be used, followed by using checkstyle to see if there are any formatting errors. To help review this PR it might be useful to do something similar and check individual commits. For this reason, it would likely not be useful to check the diff presented on GitHub since it's been rebased to be ahead of master.

Follow Up

  1. I'm not sure whether this PR should directly override the tutorial-add-remark branch. If that's the case, rather than merging based at tutorial-add-remark current HEAD, it would probably be better to force tutorial-add-remark to be reset to master's HEAD before merging, to avoid any remnants of old code persisting. I think an alternative is to create some other updated branch that branches of master's current head.
  2. If we override the existing tutorial-add-remark branch completely, then this PR should not be merged until the corresponding tutorial has been updated, else the tutorial would link to commits that don't exist. The tutorial links to commits likely also need not be modified until this PR is finalized, since every rebase would change the commit's hash.

j-lum added 9 commits October 30, 2024 15:22
Add a minimum viable implementation of the `remark` command. This
implementation meant to be a bridging step between seeing a positive
result and doing things the right way.

Tests are not implemented because students are not expected to know how
to write them yet.
Add a RemarkCommand that just throws an Exception.

Add RemarkCommandTest that will test that executeUndoableCommand()
throws an Exception.

Modify AddressBookParser to accept a RemarkCommand.

Add new test to AddressBookParserTest, which will test that typing
"remark" returns an instance of RemarkCommand.
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.

This commit has been significantly modified when rebased in October 2024.
* Fix new project structure affecting Messages import statement
* Fix import formatting affecting checkstyle
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 label with any random text inside PersonListCard.fxml

Add FXML annotation in PersonCard to tie the variable to the actual
label.
Add Person#getRemark()

Modify PersonBuilder to support Remark.

Modify AddCommand to create a Person with a blank Remark.

Modify EditCommand to pass Remarks on unchanged.

Modify SampleDataUtil to add remarks for the sample data.

Add withRemark() to PersonBuilder.

This commit has been significantly modified when rebased in October
2024.
* Fix PersonBuilder to use ToStringBuilder util method
* Fix failing toStringMethod test case
Update tests and test data to use the remark field.

This commit has been significantly modified when rebased in October 2024.
* Fix json data to use "tags" instead of "tagged" as key
* Fix JsonAdaptedPerson to use tags as variable name
Replace the error message with the actual logic to modify the remarks
of a person.

Update RemarkCommandTest to test that execute() logic works.

This commit has been significantly modified when rebased in October 2024.
* Fix new project structure affecting Messages import statement
Copy link

canihasreview bot commented Oct 30, 2024

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

@damithc
Copy link
Contributor

damithc commented Nov 2, 2024

Thanks for this PR, @drustanyjt
We'll try to get this merged before the next semester starts.
BTW, would it be cleaner to PR against the master branch so that only the 9 original commits appear in the PR? That way, we can accept this new branch and delete the old branch.

@drustanyjt drustanyjt changed the base branch from tutorial-add-remark to master November 2, 2024 19:42
@drustanyjt
Copy link
Author

@damithc that makes sense, I've edited the PR to reference master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants