Exception handling (#207)

So this PR is a fix for most things that I found which could be fixed without major refactoring, including: 1. Specifying the backup path to a file (via prompt and `--new_path`). 2. KeyboardInterrupts at menus and prompts. 3. Failed but installed commands ran while backing up packages outputted error messages to STDOUT. This was the case for cargo's backup which ran `ls xxx/.cargo/bin` when I didn't have cargo. 4. There was a `configs_mapping` instead of `config_mapping` in `reinstall_configs()` when accessing the config dict. 5. Minor refactoring so that not installed packages won't get a backup directory so we don't need a for loop to clean those up. 6. Reinstalling packages without package backups errored out so now you get prompted to backup packages first. I tried to follow your lead of allowing users to fix the issues themselves by reprompting and things like that but if you have any further suggestions I'll try my best to do them. For some of the unfixed problems (critical and otherwise) like failing to reinstall configs for an already existing Sublime Text 3 because copytree() doesn't overwrite, I'll make a separate issue. The main reason for not fixing them in this PR was just because I didn't want to do major code revisions, I just wanted to focus on general handling in this one.
alichtman · Jan 2, 2019 · 9f8f9d4 · 9f8f9d4
1 parent 4214d47
commit 9f8f9d4
Show file tree

Hide file tree

Showing 7 changed files with 83 additions and 42 deletions.
diff --git a/shallow_backup/__main__.py b/shallow_backup/__main__.py
@@ -3,7 +3,9 @@
 from .prompts import *
 from .reinstall import *
 from .git_wrapper import *
-from .utils import mkdir_warn_overwrite, destroy_backup_dir, expand_to_abs_path
+from .utils import (
+	mkdir_warn_overwrite, destroy_backup_dir, expand_to_abs_path,
+	new_dir_is_valid)
 
 
 # custom help options
@@ -64,6 +66,10 @@ def cli(show, all, dotfiles, configs, packages, fonts, old_path, new_path, remot
 	# User entered a new path, so update the config
 	if new_path:
 		abs_path = os.path.abspath(new_path)
+
+		if not new_dir_is_valid(abs_path):
+			sys.exit(1)
+
 		print_path_blue("\nUpdating shallow-backup path to:", abs_path)
 		backup_config["backup_path"] = abs_path
 		write_config(backup_config)

diff --git a/shallow_backup/backup.py b/shallow_backup/backup.py
@@ -120,17 +120,16 @@ def backup_packages(backup_path, skip=False):
 	print_pkg_mgr_backup("npm")
 	command = "npm ls --global --parseable=true --depth=0"
 	temp_file_path = "{}/npm_temp_list.txt".format(backup_path)
-	run_cmd_write_stdout(command, temp_file_path)
-	npm_dest_file = "{0}/npm_list.txt".format(backup_path)
-	# Parse npm output
-	with open(temp_file_path, mode="r+") as temp_file:
-		# Skip first line of file
-		temp_file.seek(1)
-		with open(npm_dest_file, mode="w+") as dest:
-			for line in temp_file:
-				dest.write(line.split("/")[-1])
-
-	os.remove(temp_file_path)
+	if not run_cmd_write_stdout(command, temp_file_path):
+		npm_dest_file = "{0}/npm_list.txt".format(backup_path)
+		# Parse npm output
+		with open(temp_file_path, mode="r+") as temp_file:
+			# Skip first line of file
+			temp_file.seek(1)
+			with open(npm_dest_file, mode="w+") as dest:
+				for line in temp_file:
+					dest.write(line.split("/")[-1])
+		os.remove(temp_file_path)
 
 	# atom package manager
 	print_pkg_mgr_backup("Atom")
@@ -151,11 +150,6 @@ def backup_packages(backup_path, skip=False):
 	dest = "{}/system_apps_list.txt".format(backup_path)
 	run_cmd_write_stdout(command, dest)
 
-	# Clean up empty package list files
-	for file in get_abs_path_subfiles(backup_path):
-		if os.path.getsize(file) == 0:
-			os.remove(file)
-
 
 def backup_fonts(backup_path, skip=False):
 	"""

diff --git a/shallow_backup/git_wrapper.py b/shallow_backup/git_wrapper.py
@@ -97,14 +97,15 @@ def move_git_repo(source_path, dest_path):
 	dest_git_ignore = os.path.join(dest_path, '.gitignore')
 	git_exists = os.path.exists(dest_git_dir)
 	gitignore_exists = os.path.exists(dest_git_ignore)
+
 	if git_exists or gitignore_exists:
 		print_red_bold("Evidence of a git repo has been detected.")
 		if git_exists:
 			print_path_red("A git repo already exists here:", dest_git_dir)
 		if gitignore_exists:
 			print_path_red("A gitignore file already exists here:", dest_git_ignore)
 		print_red_bold("Exiting to prevent accidental deletion of user data.")
-		sys.exit()
+		sys.exit(1)
 
 	git_dir = os.path.join(source_path, '.git')
 	git_ignore_file = os.path.join(source_path, '.gitignore')
@@ -115,3 +116,4 @@ def move_git_repo(source_path, dest_path):
 		print_blue_bold("Moving git repo to new location.")
 	except FileNotFoundError:
 		pass
+
diff --git a/shallow_backup/printing.py b/shallow_backup/printing.py
@@ -1,3 +1,4 @@
+import sys
 import inquirer
 from colorama import Fore, Style
 from .constants import ProjInfo
@@ -83,23 +84,27 @@ def print_section_header(title, color):
 
 
 def print_pkg_mgr_backup(mgr):
-	print("{}Backing up {}{}{}{}{} packages list...{}".format(Fore.BLUE, Style.BRIGHT, Fore.YELLOW, mgr, Fore.BLUE,
-	                                                          Style.NORMAL, Style.RESET_ALL))
+	print("{}Backing up {}{}{}{}{} packages list...{}".format(Fore.BLUE, Style.BRIGHT, Fore.YELLOW, mgr,
+															  Fore.BLUE, Style.NORMAL, Style.RESET_ALL))
 
 
 def print_pkg_mgr_reinstall(mgr):
-	print("{}Reinstalling {}{}{}{}{}...{}".format(Fore.BLUE, Style.BRIGHT, Fore.YELLOW, mgr, Fore.BLUE, Style.NORMAL, Style.RESET_ALL))
+	print("{}Reinstalling {}{}{}{}{}...{}".format(Fore.BLUE, Style.BRIGHT, Fore.YELLOW,
+												  mgr, Fore.BLUE, Style.NORMAL, Style.RESET_ALL))
 
 
-def prompt_yes_no(message, color):
+def prompt_yes_no(message, color, invert=False):
 	"""
 	Print question and return True or False depending on user selection from list.
 	"""
 	questions = [inquirer.List('choice',
 	                           message=color + Style.BRIGHT + message + Fore.BLUE,
-	                           choices=[' Yes', ' No'],
+	                           choices=(' No', ' Yes') if invert else (' Yes', ' No')
 	                           )
 	             ]
 
 	answers = inquirer.prompt(questions)
-	return answers.get('choice').strip().lower() == 'yes'
+	if answers:
+		return answers.get('choice').strip().lower() == 'yes'
+	else:
+		sys.exit(1)
diff --git a/shallow_backup/prompts.py b/shallow_backup/prompts.py
@@ -1,10 +1,12 @@
 import os
+import sys
 import inquirer
 from colorama import Fore, Style
 from .utils import *
 from .printing import *
 from .config import *
 from .git_wrapper import git_set_remote, move_git_repo
+from .utils import new_dir_is_valid
 
 
 def path_update_prompt(config):
@@ -14,14 +16,20 @@ def path_update_prompt(config):
 	"""
 	current_path = config["backup_path"]
 	print_path_blue("Current shallow-backup path:", current_path)
-	if prompt_yes_no("Would you like to update this?", Fore.GREEN):
-		print_green_bold("Enter relative or absolute path:")
-		abs_path = expand_to_abs_path(input())
-		print_path_blue("\nUpdating shallow-backup path to:", abs_path)
-		mkdir_warn_overwrite(abs_path)
-		move_git_repo(current_path, abs_path)
-		config["backup_path"] = abs_path
-		write_config(config)
+	if prompt_yes_no("Would you like to update this?", Fore.GREEN, invert=True):
+		while True:
+			print_green_bold("Enter relative or absolute path:")
+			abs_path = expand_to_abs_path(input())
+
+			if not new_dir_is_valid(abs_path):
+				continue
+
+			print_path_blue("\nUpdating shallow-backup path to:", abs_path)
+			mkdir_warn_overwrite(abs_path)
+			move_git_repo(current_path, abs_path)
+			config["backup_path"] = abs_path
+			write_config(config)
+			return
 
 
 def git_url_prompt(repo):
@@ -159,4 +167,9 @@ def main_menu_prompt():
 	             ]
 
 	answers = inquirer.prompt(questions)
-	return answers.get('choice').strip().lower()
+
+	if answers:
+		return answers.get('choice').strip().lower()
+	else:
+		# KeyboardInterrupts
+		sys.exit(1)
diff --git a/shallow_backup/reinstall.py b/shallow_backup/reinstall.py
@@ -1,7 +1,7 @@
 import os
 from shlex import quote
 from colorama import Fore
-from .utils import run_cmd, get_abs_path_subfiles
+from .utils import run_cmd, get_abs_path_subfiles, empty_backup_dir_check
 from .printing import *
 from .compatibility import *
 from .config import get_config
@@ -15,7 +15,9 @@ def reinstall_dots_sb(dots_path):
 	"""
 	Reinstall all dotfiles and folders by copying them to the home dir.
 	"""
+	empty_backup_dir_check(dots_path, 'dotfile')
 	print_section_header("REINSTALLING DOTFILES", Fore.BLUE)
+
 	home_path = os.path.expanduser('~')
 	for file in get_abs_path_subfiles(dots_path):
 		if os.path.isdir(file):
@@ -29,7 +31,9 @@ def reinstall_fonts_sb(fonts_path):
 	"""
 	Reinstall all fonts.
 	"""
+	empty_backup_dir_check(fonts_path, 'font')
 	print_section_header("REINSTALLING FONTS", Fore.BLUE)
+
 	# Copy every file in fonts_path to ~/Library/Fonts
 	for font in get_abs_path_subfiles(fonts_path):
 		font_lib_path = get_fonts_dir()
@@ -42,15 +46,13 @@ def reinstall_configs_sb(configs_path):
 	"""
 	Reinstall all configs from the backup.
 	"""
+	empty_backup_dir_check(configs_path, 'config')
 	print_section_header("REINSTALLING CONFIG FILES", Fore.BLUE)
 
-	def backup_prefix(path):
-		return os.path.join(configs_path, path)
-
 	config = get_config()
-	for dest_path, backup_loc in config["configs_mapping"].items():
+	for dest_path, backup_loc in config["config_mapping"].items():
 		dest_path = quote(dest_path)
-		path_to_backup = quote(backup_prefix(backup_loc))
+		path_to_backup = quote(os.path.join(configs_path, backup_loc))
 		# TODO: REFACTOR WITH GENERIC COPY FUNCTION.
 		if os.path.isdir(path_to_backup):
 			copytree(path_to_backup, dest_path)
@@ -64,6 +66,7 @@ def reinstall_packages_sb(packages_path):
 	"""
 	Reinstall all packages from the files in backup/installs.
 	"""
+	empty_backup_dir_check(packages_path, 'package')
 	print_section_header("REINSTALLING PACKAGES", Fore.BLUE)
 
 	# Figure out which install lists they have saved

diff --git a/shallow_backup/utils.py b/shallow_backup/utils.py
@@ -12,10 +12,10 @@ def run_cmd(command):
 	"""
 	try:
 		if not isinstance(command, list):
-			process = sp.run(command.split(), stdout=sp.PIPE)
+			process = sp.run(command.split(), stdout=sp.PIPE, stderr=sp.DEVNULL)
 			return process
 		else:
-			process = sp.run(command, stdout=sp.PIPE)
+			process = sp.run(command, stdout=sp.PIPE, stderr=sp.DEVNULL)
 			return process
 	except FileNotFoundError:  # If package manager is missing
 		return None
@@ -25,11 +25,23 @@ def run_cmd_write_stdout(command, filepath):
 	"""
 	Runs a command and then writes its stdout to a file
 	:param: command str representing command to run
+	:param: filepath str file to write command's stdout to
 	"""
 	process = run_cmd(command)
-	if process:
+	if process and process.returncode == 0:
 		with open(filepath, "w+") as f:
 			f.write(process.stdout.decode('utf-8'))
+	else:
+		print_path_red("An error occurred while running: $", command)
+		return 1
+
+
+def new_dir_is_valid(abs_path):
+	if os.path.isfile(abs_path):
+		print_path_red('New path is a file:', abs_path)
+		print_red_bold('Please enter a directory.\n')
+		return False
+	return True
 
 
 def safe_mkdir(directory):
@@ -66,6 +78,12 @@ def mkdir_warn_overwrite(path):
 		print_path_blue("Created directory:", path)
 
 
+def empty_backup_dir_check(backup_path, backup_type):
+	if not os.path.isdir(backup_path) or not os.listdir(backup_path):
+		print_red_bold('No {} backup found.'.format(backup_type))
+		sys.exit(1)
+
+
 def destroy_backup_dir(backup_path):
 	"""
 	Deletes the backup directory and its content