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.

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

Fix npm install #293

Closed
wants to merge 3 commits into from
Closed

Conversation

tim-coutinho
Copy link
Contributor

This addresses #148.

@alichtman
Copy link
Owner

Think it's reasonable to put together a test to make sure this bug doesn't show up again?

@tim-coutinho
Copy link
Contributor Author

Want me to refactor test_reinstall_dotfiles to test_reinstall and throw it in there, or have a separate test file for reinstalling packages?

@alichtman
Copy link
Owner

Let's do a separate test file for that.

shallow_backup/utils.py Outdated Show resolved Hide resolved
f.write(process.stdout.decode('utf-8'))
elif process:
print_path_red("An error occurred while running: $", command)
if process:
Copy link
Owner

Choose a reason for hiding this comment

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

Let's revert this conditional back to the way it was before. I think that was easier to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you sure? I changed it to this so that it would differentiate between a package manager not existing on the system and an actual error occurring. As is, if you don't have any package managers, you're getting a bunch of red-colored errors spit out at you rather than a simple warning saying it doesn't exist.

@alichtman
Copy link
Owner

Nice fix. Few small changes and I'll merge this in.

shallow_backup/utils.py Outdated Show resolved Hide resolved
@alichtman
Copy link
Owner

Addressed in #299

@alichtman alichtman closed this Dec 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants