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

Refactor person to contact #27

Merged

Conversation

CJ-Lee01
Copy link

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

Updated a few test cases and documentation to reflect the change and changed toString method to pass test cases.

@CJ-Lee01 CJ-Lee01 added the type.Chore Something to do but not a story, feature, bug or documentation (CS2103T label) label Oct 5, 2023
@CJ-Lee01 CJ-Lee01 requested review from a team, McNaBry and wamps-jp and removed request for a team October 5, 2023 07:12
@CJ-Lee01 CJ-Lee01 enabled auto-merge October 5, 2023 09:05
@CJ-Lee01 CJ-Lee01 linked an issue Oct 7, 2023 that may be closed by this pull request
@CJ-Lee01
Copy link
Author

CJ-Lee01 commented Oct 7, 2023

It appears that the JsonSerializableAddressBook class cannot be refactored without breaking a lot of things.

@wxwern
Copy link

wxwern commented Oct 7, 2023

It appears that the JsonSerializableAddressBook class cannot be refactored without breaking a lot of things.

I believe this is because that class (and other similar ones) functions as the interface between the final JSON file stored and the internal models, converting to and from via Jackson. Since many tests use template JSON files to verify JSON Serializable class behaviors, all JSON test cases must be manually modified and likely cannot be automatically refactored by the IDE.

Copy link

@McNaBry McNaBry left a comment

Choose a reason for hiding this comment

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

Overall looks good to me. The changes pass the tests on my side as well.

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.

Looks okay. We may need to change the name of the person package to contact later on, but for now it should be fine!

@CJ-Lee01 CJ-Lee01 merged commit 4a43eaf into AY2324S1-CS2103T-W08-3:master Oct 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type.Chore Something to do but not a story, feature, bug or documentation (CS2103T label)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor Person to Contact
4 participants