Skip to content
This repository has been archived by the owner on Mar 5, 2023. It is now read-only.

Storage: Add backupAddressBook(ReadOnlyAddressBook) #594

Closed
wants to merge 1 commit into from

Conversation

yamgent
Copy link
Member

@yamgent yamgent commented Jul 28, 2017

Part of #784. Rebased, fixed the compilation error.

Proposed commit message:

Storage saves to a single file path.

Users may want to do a backup of the address book in a different
location.

Let's add Storage#backupAddressBook(ReadOnlyAddressBook).

Note: This PR is used as an example for future developers. Do not merge!

@mention-bot
Copy link

@yamgent, thanks for your PR! By analyzing the history of the files in this pull request, we identified @damithc, @yl-coder and @chao1995 to be potential reviewers.

@yamgent yamgent changed the title Storage: Add backupAddressBook() Storage: Add backupAddressBook(ReadOnlyAddressBook) Jul 28, 2017
@yamgent yamgent closed this Jul 29, 2017
@yamgent yamgent added the pr.Answer Pull request is answer for a tutorial in developer guide. Do not actually merge. label Jan 20, 2018
@yamgent yamgent reopened this Jan 20, 2018
@yamgent yamgent force-pushed the getting-started-storage branch 2 times, most recently from 8e692f1 to a95b7df Compare January 20, 2018 02:30
@CanIHasReview-bot
Copy link

v1

@yamgent submitted v1 for review.

(📚 Archive)

Checkout this PR version locally
git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/594/1/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

Copy link
Contributor

@Zhiyuan-Amos Zhiyuan-Amos left a comment

Choose a reason for hiding this comment

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

As spoken, rmb to update https://github.com/se-edu/addressbook-level4/blob/master/docs/DeveloperGuide.adoc#storage-component to update the logic in XmlAddressBookStorage too.

@@ -77,6 +77,10 @@ public void saveAddressBook(ReadOnlyAddressBook addressBook, String filePath) th
addressBookStorage.saveAddressBook(addressBook, filePath);
}

@Override
public void backupAddressBook(ReadOnlyAddressBook addressBook) throws IOException {
saveAddressBook(addressBook, addressBookStorage.getAddressBookFilePath() + "-backup.xml");
Copy link
Contributor

Choose a reason for hiding this comment

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

This method should be calling addressBookStorage#backupAddressBook(ReadOnlyAddressBook) instead :P

@yamgent yamgent force-pushed the getting-started-storage branch 3 times, most recently from 9229461 to 9356bf3 Compare February 8, 2018 04:39
@CanIHasReview-bot
Copy link

v2

@yamgent submitted v2 for review.

(📚 Archive) (📈 Interdiff between v1 and v2)

Checkout this PR version locally
git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/594/2/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

Copy link
Contributor

@Zhiyuan-Amos Zhiyuan-Amos left a comment

Choose a reason for hiding this comment

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

💯

@Zhiyuan-Amos
Copy link
Contributor

@yamgent this PR is failing 1 test :<

@CanIHasReview-bot
Copy link

v3

@yamgent submitted v3 for review.

(📚 Archive) (📈 Interdiff between v2 and v3) (📈 Range-Diff between v2 and v3)

Checkout this PR version locally
git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/594/3/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

@yamgent yamgent requested a review from a team August 9, 2018 05:47
@yamgent
Copy link
Member Author

yamgent commented Aug 9, 2018

Changelog:

  • Because we changed the way we handled paths (from String to Path), we must now use the Paths class to generate the backup file path:

saveAddressBook(addressBook, Paths.get(filePath.toString() + ".backup"));

@@ -60,6 +61,11 @@ public Path getAddressBookFilePath() {
}
}

@Override
public void backupAddressBook(ReadOnlyAddressBook addressBook) throws IOException {
saveAddressBook(addressBook, Paths.get(filePath.toString() + ".backup"));
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering whether the backup Path should be stored in a private final field, to be assigned in the constructor.

This is so that we don't have to call Paths.get whenever this method is called.

@CanIHasReview-bot
Copy link

v4

@yamgent submitted v4 for review.

(📚 Archive) (📈 Interdiff between v3 and v4) (📈 Range-Diff between v3 and v4)

Checkout this PR version locally
git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/594/4/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

@yamgent yamgent requested a review from a team August 18, 2018 12:20

public XmlAddressBookStorage(Path filePath) {
this.filePath = filePath;
this.backupFilePath = Paths.get(filePath.toString() + ".backup");
Copy link
Member Author

Choose a reason for hiding this comment

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

Shouldn't use this. here, my bad.

Storage saves to a single file path.

Users may want to do a backup of the address book in a different
location.

Let's add Storage#backupAddressBook(ReadOnlyAddressBook).
@CanIHasReview-bot
Copy link

v5

@yamgent submitted v5 for review.

(📚 Archive) (📈 Interdiff between v4 and v5) (📈 Range-Diff between v4 and v5)

Checkout this PR version locally
git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/594/5/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

@yamgent yamgent requested a review from damithc August 19, 2018 02:11
@damithc damithc closed this Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr.Answer Pull request is answer for a tutorial in developer guide. Do not actually merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants