Add remove backup dir functionality #101

neequole · 2018-10-19T05:40:44Z

alichtman

Great work, @neequole. We can get this feature merged in after a few minor changes. See the comments below.

alichtman · 2018-10-19T05:44:58Z

shallow_backup.py

@@ -657,6 +657,13 @@ def prompt_for_path_update(config):
 		move_git_folder_to_path(current_path, abs_path)


+def clean_backup_dir(backup_path):


Can you add a docstring?

alichtman · 2018-10-19T05:48:46Z

shallow_backup.py

+def clean_backup_dir(backup_path):
+	try:
+		shutil.rmtree(backup_path)
+		print("{} Deleted backup directory {} {}".format(Fore.BLUE, backup_path, Style.RESET_ALL))


We should use Fore.RED and Style.BRIGHT here since removal is a "dangerous" action.

I usually put print statements before the action, and use the infinitive tense. "Deleting" and then I finish the line with ellipsis.

alichtman · 2018-10-19T05:53:29Z

shallow_backup.py

@@ -689,6 +697,13 @@ def cli(complete, dotfiles, configs, packages, fonts, old_path, new_path, remote
 		print(Fore.RED + Style.BRIGHT +
 		      "Removed config file..." + Style.RESET_ALL)
 		sys.exit()
+	elif clean:
+		backup_home_path = get_config()["backup_path"]
+		if prompt_yes_no("Erase backup directory {}?".format(backup_home_path), Fore.RED):


I've gone back and forth about actually prompting here. If we get to this point in the program, the user either used the -destroy_backup flag, or hit "Destroy backup" in the menu. That's a pretty deliberate action.

I'm not entirely sure we need to prompt here. What are your thoughts?

Side thoughts

Let's change the flag to -destroy_backup since that's a lot clearer.

Then we should refactor the other method names as needed.

Let's add this option to the menu.

@alichtman Do you mean removing the prompt and just go straightly on the delete operation? My thought was that for any dangerous action, we have to double confirm it to user that is why I added the prompt. Final thoughts?

For your side thoughts no. 3, do you mean adding it after this?

elif selection == "reinstall configs": reinstall_config_files(configs_path) # Add here elif selection == "Destroy backups": destroy_backup() # Can you advise if we need to exit at this point or we can go on the git stuffs below? sys.exit() git_add_all_commit(repo, backup_home_path) git_push_if_possible(repo) sys.exit()

My thought was that for any dangerous action, we have to double confirm it to user that is why I added the prompt.

Alright, I have a good solution. We only prompt if they select to remove the backup from the CLI. If they remove with the flag, do that without a double confirmation. So, the process will be slightly different based on how the user selects to remove the backup.

For your side thoughts no. 3, do you mean adding it after this?

I meant that we should add it to the menu of options people can choose from in the CLI.

* Rename clean flag to destroy_backup * Rename method clean_backup_dir to destroy_backup_dir * Add the destroy backup option to the menu * Minor CLI message changes

…with flag

ntibay · 2018-10-19T12:18:54Z

@alichtman Updated based on your review. Pls check thanks!

alichtman

Awesome, a couple quick changes...

alichtman · 2018-10-19T12:16:53Z

shallow_backup.py

@@ -109,7 +109,8 @@ def backup_prompt():
                                        ' Back up fonts',
 	                                    ' Back up everything',
                                        ' Reinstall configs',
-	                                    ' Reinstall packages'
+	                                    ' Reinstall packages',
+										' Destroy backups'


Suggested change

' Destroy backups'

' Destroy backup'

alichtman · 2018-10-19T12:17:47Z

shallow_backup.py

@@ -774,6 +790,13 @@ def cli(complete, dotfiles, configs, packages, fonts, old_path, new_path, remote
 			reinstall_package(packages_path)
 		elif selection == "reinstall configs":
 			reinstall_config_files(configs_path)
+		elif selection == "destroy backups":
+			if prompt_yes_no("Erase backup directory {}?".format(backup_home_path), Fore.RED):


Suggested change

if prompt_yes_no("Erase backup directory {}?".format(backup_home_path), Fore.RED):

if prompt_yes_no("Erase backup directory: {}?".format(backup_home_path), Fore.RED):

alichtman · 2018-10-19T12:18:55Z

tests/test_deletes.py

+from shallow_backup import destroy_backup_dir
+
+BACKUP_DIR = 'shallow-backup-test-copy-backup-dir'
+TEST_BACKUP_TEXT_FILE = BACKUP_DIR + '/test-file.txt'


Use os.path.join here instead of hardcoding the path. Windows compatibility is in the works.

alichtman · 2018-10-19T12:20:35Z

Pull from master to resolve those merge conflicts.

neequole · 2018-10-19T22:45:24Z

@alichtman Changes applied :>

alichtman · 2018-10-19T23:45:19Z

Looks good to me.

ntibay added 4 commits October 19, 2018 16:12

Add deleting of backup directory functionality

88b498a

Test clean backup directory method

d18f71b

Add file to backup test directory

716f6b5

Update contributors and readme files

d7140f3

alichtman requested changes Oct 19, 2018

View reviewed changes

ntibay added 2 commits October 19, 2018 23:11

Add changes based on QA

93d0c67

* Rename clean flag to destroy_backup * Rename method clean_backup_dir to destroy_backup_dir * Add the destroy backup option to the menu * Minor CLI message changes

Change option from Destroy backups to destroy backup for consistency …

5ff3f66

…with flag

alichtman requested changes Oct 19, 2018

View reviewed changes

ntibay added 2 commits October 20, 2018 09:09

Use os.path.join for creating file path

5ffa0f7

Resolve merge conflicts

181eae2

alichtman approved these changes Oct 19, 2018

View reviewed changes

alichtman merged commit 0da0e6a into alichtman:master Oct 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add remove backup dir functionality #101

Add remove backup dir functionality #101

neequole commented Oct 19, 2018

alichtman left a comment

alichtman Oct 19, 2018

alichtman Oct 19, 2018

alichtman Oct 19, 2018 •

edited

Loading

ntibay Oct 19, 2018

alichtman Oct 19, 2018

ntibay commented Oct 19, 2018

alichtman left a comment •

edited

Loading

alichtman Oct 19, 2018

alichtman Oct 19, 2018

alichtman Oct 19, 2018

alichtman commented Oct 19, 2018

neequole commented Oct 19, 2018

alichtman commented Oct 19, 2018

		@@ -657,6 +657,13 @@ def prompt_for_path_update(config):
		move_git_folder_to_path(current_path, abs_path)


		def clean_backup_dir(backup_path):

	if prompt_yes_no("Erase backup directory {}?".format(backup_home_path), Fore.RED):
	if prompt_yes_no("Erase backup directory: {}?".format(backup_home_path), Fore.RED):

Add remove backup dir functionality #101

Add remove backup dir functionality #101

Conversation

neequole commented Oct 19, 2018

alichtman left a comment

Choose a reason for hiding this comment

alichtman Oct 19, 2018

Choose a reason for hiding this comment

alichtman Oct 19, 2018

Choose a reason for hiding this comment

alichtman Oct 19, 2018 • edited Loading

Choose a reason for hiding this comment

ntibay Oct 19, 2018

Choose a reason for hiding this comment

alichtman Oct 19, 2018

Choose a reason for hiding this comment

ntibay commented Oct 19, 2018

alichtman left a comment • edited Loading

Choose a reason for hiding this comment

alichtman Oct 19, 2018

Choose a reason for hiding this comment

alichtman Oct 19, 2018

Choose a reason for hiding this comment

alichtman Oct 19, 2018

Choose a reason for hiding this comment

alichtman commented Oct 19, 2018

neequole commented Oct 19, 2018

alichtman commented Oct 19, 2018

alichtman Oct 19, 2018 •

edited

Loading

alichtman left a comment •

edited

Loading