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.

Sign up for GitHub

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

Jump to bottom

Exception handling #207

Merged
merged 31 commits into from
Jan 2, 2019
Merged

Exception handling #207

merged 31 commits into from
Jan 2, 2019

Conversation

ibokuri
Copy link

@ibokuri ibokuri commented Dec 27, 2018

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.

Jason Phan added 5 commits December 26, 2018 08:26
Exception handling for backups (#206)
5b47b3c 
This commit covers:
  1. Specifying the backup path to an existing filename.
  2. KeyboardInterrupts at menus and prompts.
  3. When backing up packages, failed but installed commands (e.g., ls xxx/.cargo/bin) outputted their error messages to STDOUT.
  4. No need to clean up empty backup files now since we don't write to the packages' files if they're not installed.
  5. Because of #4, we needed to move npm's 2nd write section into an if so that it wouldn't try to remove a nonexistent file.
Exception handling for --new_path (#206)
1d279fd 
I decided to exit here instead of reprompting like in path_update_prompt() because if you're using the
--new_path option you decided not to just use the regular path update prompt for some reason. I don't
really know why this option's a thing honestly because running shallow-backup with no arguments always
prompts you to update the path anyways.
Typo in config dict key
59ba373
Fixed reinstalling packages w/ no package backups
a9b1956 
Also got rid of unnecessary inner function.
coverage: trailing space
ecbfbe5
Repository owner deleted a comment Dec 27, 2018
Repository owner deleted a comment Dec 27, 2018
Repository owner deleted a comment Dec 27, 2018
Repository owner deleted a comment from ibokuri Dec 27, 2018
Repository owner deleted a comment Dec 27, 2018
@ibokuri
Copy link
Author

ibokuri commented Dec 27, 2018 •
edited
Loading

why was a 49.451% coverage acceptable but not 49.351

@alichtman
Copy link
Owner

alichtman commented Dec 27, 2018 •
edited
Loading

(On mobile, sorry for the potentially awful formatting.)

  1. This is a great improvement. Idk why I never considered any of this...

  2. I have to do a code review on a laptop and I didn't bring mine on vacation. If I haven't said anything by Jan. 3rd, drop a comment here to remind me.

  3. Yeah, Codacy is a pain sometimes. It gives useful feedback + draws attention to potential vulnerabilities sometimes, so I keep it around, but don't worry about all the issues it flags. I'll take a look at the dashboard when I'm home and see if there's anything really important that should be taken care of. Safe to ignore the rest.

  4. Same goes for the code coverage metrics. It only flagged your PR because the coverage % decreased. If we can mock keystrokes, then this is all testable and we probably should have tests for it (and Coverall will approve the PR on a testing basis). I hadn't figured out a way to do it, but maybe you could take a look?

In summary, this looks awesome and I can't wait to get this merged in. I'll go through it when I'm home and see if there's anything we can improve. :)

@ibokuri
Copy link
Author

ibokuri commented Dec 27, 2018

Sounds good! No rush though enjoy your vacation! 😄

alichtman
alichtman requested changes Jan 1, 2019
View reviewed changes
Copy link
Owner

@alichtman alichtman left a comment

Choose a reason for hiding this comment

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

Overall, great improvements to the user experience.

Minor refactoring is needed to minimize code duplication. See other comments for further feedback.

I'll cut a new release as soon as we get this merged in.

shallow_backup/__main__.py Show resolved Hide resolved
shallow_backup/utils.py Outdated Show resolved Hide resolved
shallow_backup/utils.py Show resolved Hide resolved
shallow_backup/reinstall.py Outdated Show resolved Hide resolved
shallow_backup/backup.py Outdated Show resolved Hide resolved
shallow_backup/git_wrapper.py Outdated Show resolved Hide resolved
shallow_backup/printing.py Outdated Show resolved Hide resolved
shallow_backup/prompts.py Outdated Show resolved Hide resolved
shallow_backup/prompts.py Outdated Show resolved Hide resolved
shallow_backup/utils.py Outdated Show resolved Hide resolved
Repository owner deleted a comment Jan 1, 2019
alichtman
alichtman reviewed Jan 1, 2019
View reviewed changes
shallow_backup/utils.py Outdated Show resolved Hide resolved
Repository owner deleted a comment Jan 1, 2019
Jason Phan and others added 2 commits January 1, 2019 17:06
Repository owner deleted a comment Jan 1, 2019
Repository owner deleted a comment Jan 1, 2019
@ibokuri
Copy link
Author

ibokuri commented Jan 2, 2019

Any ideas about how to do this?

No unfortunately :/

@alichtman
Copy link
Owner

alichtman commented Jan 2, 2019 •
edited
Loading

No unfortunately :/

No big deal, testing exception handling isn't super critical. I also have no idea how to do this.

I'm going to sit on this for a bit before I merge it to make sure I'm not missing anything, but I'm pretty confident it's all done.

It's been a pleasure working with you on this.

Thank you for the contribution.

Repository owner deleted a comment Jan 2, 2019
Repository owner deleted a comment Jan 2, 2019
Repository owner deleted a comment Jan 2, 2019
@ibokuri
Copy link
Author

ibokuri commented Jan 2, 2019

It's been a pleasure to work with you as well, thanks alot for the fun!

alichtman
alichtman reviewed Jan 2, 2019
View reviewed changes
shallow_backup/utils.py Outdated Show resolved Hide resolved
alichtman
alichtman reviewed Jan 2, 2019
View reviewed changes
shallow_backup/utils.py Outdated Show resolved Hide resolved
Repository owner deleted a comment Jan 2, 2019
Repository owner deleted a comment Jan 2, 2019
Repository owner deleted a comment Jan 2, 2019
Repository owner deleted a comment Jan 2, 2019
alichtman
alichtman reviewed Jan 2, 2019
View reviewed changes
shallow_backup/utils.py Outdated Show resolved Hide resolved
Repository owner deleted a comment Jan 2, 2019
Repository owner deleted a comment Jan 2, 2019
Repository owner deleted a comment Jan 2, 2019
Repository owner deleted a comment Jan 2, 2019
Repository owner deleted a comment Jan 2, 2019
Repository owner deleted a comment Jan 2, 2019
Repository owner deleted a comment Jan 2, 2019
@alichtman alichtman merged commit 9f8f9d4 into alichtman:master Jan 2, 2019
@ibokuri ibokuri deleted the exception_handling branch January 2, 2019 05:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alichtman alichtman alichtman approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ibokuri @alichtman