search

se-edu / guides

Style guides and tutorials for SE student projects
MIT License
7 stars 26 forks source link

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

Closed mfjkri closed 1 month ago

mfjkri commented 1 month ago

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: https://github.com/mfjkri/addressbook-level3/commit/3e658057198ca9e1ff0fce2ce1b353df78e0be86

Carlintyj commented 1 month ago

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 commented 1 month ago

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.

gongg21 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

Thanks for this PR @mfjkri I've adapted its content in https://github.com/se-edu/guides/commit/a96248617b5ea94c857a8f057ddee6f93d3e47ca