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

[AB3: Removing Fields] Add git diff for removing address field #86

Closed

Conversation

mfjkri
Copy link
Contributor

@mfjkri mfjkri commented Sep 23, 2024

Fixes #84
I've added a git diff to compare the changes made when removing the address field.

Currently, the commit is on a branch in my fork of the AB3 repository, as I don't have permission to create a new remote branch on the main repository. It can cherry-picked over later after a branch is created for it and I will update the commit link again after.

Commit link: mfjkri/addressbook-level3@3e65805

@Carlintyj
Copy link

Carlintyj commented Sep 24, 2024

Hello Fikri, I have looked at the issue and the PR and agree that this would be a useful addition to the guide. In addition, I have tried out your commit and the program seems to be able to compile, execute and run without the address field.

However, just a small nit, as your statement is in the "Tidying Up" section and directly after talking about removing the address field from the JSON files, it might be clearer to indicate that the git diff is not only for the JSON files but the whole program.

Here is a proposed change for your statement:
"After you've manually removed the address field from each relevant JSON file, you can cross-check your changes with this diff. This link provides a complete overview of all changes made throughout the guide, including the removal of the address field from various parts of ab3."

@mfjkri
Copy link
Contributor Author

mfjkri commented Sep 25, 2024

Hello Fikri, I have looked at the issue and the PR and agree that this would be a useful addition to the guide. In addition, I have tried out your commit and the program seems to be able to compile, execute and run without the address field.

However, just a small nit, as your statement is in the "Tidying Up" section and directly after talking about removing the address field from the JSON files, it might be clearer to indicate that the git diff is not only for the JSON files but the whole program.

Here is a proposed change for your statement: "After you've manually removed the address field from each relevant JSON file, you can cross-check your changes with this diff. This link provides a complete overview of all changes made throughout the guide, including the removal of the address field from various parts of ab3."

Hi Carlin, thank you for the suggestion!
I have made the edit to make it more clear.

@mfjkri mfjkri changed the title [#84] Add git diff for removing address field [AB3: Removing Fields] Add git diff for removing address field Sep 25, 2024
@gongg21
Copy link

gongg21 commented Sep 28, 2024

Good addition to the guide! Just a suggestion to include a simple description (possibly as a sub-point or dropdown panel) on how you derived the diff file using Git as it may be useful for readers to know and some of them might be curious after seeing it as well.

@mfjkri
Copy link
Contributor Author

mfjkri commented Sep 28, 2024

Good addition to the guide! Just a suggestion to include a simple description (possibly as a sub-point or dropdown panel) on how you derived the diff file using Git as it may be useful for readers to know and some of them might be curious after seeing it as well.

While I think this can be a great addition to have, I am not entirely sure how relevant it would be to include it here in this guide.

@damithc
Copy link
Contributor

damithc commented Sep 29, 2024

Thanks for this PR @mfjkri
I've adapted its content in a962486

@damithc damithc closed this Sep 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[AB3: Removing Fields] Git diff for removing address field
4 participants