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

Expand ~ as user's home directory #161

Merged
merged 6 commits into from
Oct 30, 2018
Merged

Expand ~ as user's home directory #161

merged 6 commits into from
Oct 30, 2018

Conversation

nunomdc
Copy link
Contributor

@nunomdc nunomdc commented Oct 28, 2018

Should fix issue #155.

@@ -60,6 +60,7 @@ def cli(add, rm, show, all, dotfiles, configs, packages, fonts, old_path, new_pa
print_red_bold("Removed config file...")
elif destroy_backup:
backup_home_path = get_config()["backup_path"]
backup_home_path = os.path.abspath(os.path.expanduser(backup_home_path))
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 extract os.path.abspath(os.path.expanduser(VAR)) to a function that lives in utils.py to reduce code repetition.

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.

Hey, thanks for contributing!

This solution looks like it'll solve the problem, and I can merge this in after some minor refactoring.

Here's a snippet from an article about the DRY principle in software engineering. If you're interested in learning about why it's important, you should read the full article: https://zapier.com/blog/dont-repeat-yourself/

In software engineering, DRY is the principle of reducing repetition in the code,
referring back to a single source—or "snippet"—of reusable code whenever you need it. 

Instead of repeating the same nested function calls 3/4 places, we should add a single function that accomplishes the same functionality, and just call that function instead.

After this is fixed, I'll pull this branch down for testing and merge it.

@nunomdc
Copy link
Contributor Author

nunomdc commented Oct 29, 2018

I can see where you're coming from, although if we're going to move this into a separate function then we could also explore the possibility to expand system environment variables on path inputs

@alichtman
Copy link
Owner

alichtman commented Oct 29, 2018

we could also explore the possibility to expand system environment variables on path inputs

I'm confused what you mean by this. Can you provide an example where this would be useful?

shallow_backup/utils.py Outdated Show resolved Hide resolved
@nunomdc
Copy link
Contributor Author

nunomdc commented Oct 29, 2018

we could also explore the possibility to expand system environment variables on path inputs

I'm confused what you mean by this. Can you provide an example where this would be useful?

For instance, accepting the following inputs:

  • $HOME/shallow-backup
  • $MY_BACKUP_DIR/shallow-backup

@alichtman
Copy link
Owner

alichtman commented Oct 30, 2018

Yes, that's an awesome idea. Let's do it. Created #164 so I can merge this.

@alichtman alichtman merged commit 57ed8d8 into alichtman:master Oct 30, 2018
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.

2 participants