Add -configs mode.

[schilli91]

Backup software configurations and terminal plist file and add -reinstall_configs mode to reinstall backups.

[alichtman]

Thanks for writing this feature! I've been looking forward to having this functionality for a bit and haven't had time to write it myself.

Overall, great work. A few changes/ideas I had and I'll merge this in.

1. Can you indent with tabs instead of spaces to match up with the style of the source?
2. You should add backing up the configs in the backup_all() function.
3. I have a _copy_file() function implemented already, called copy_dotfile(). You could refactor that to use just one method. I like the _copy_file name change, so def keep that.
4. I have a _copy_folder() method called copy_dotfolder(). Maybe update the name of my method to _copy_folder() and use that to do what you need.
5. Let's update the -reinstall flag, and all references to it, to be -reinstall_packages because there is now a distinction that needs to be made.
6. A nice usability thing would be adding --help to the list of click help options.

Can you take care of these?

[schilli91]

Hey, thanks for the feedback, I appreciate it (:

I introduced the new copy methods in order to refactor the old ones and make their names more appropriate for the "new" purpose of being generic functions. I find the copy_dotfolder() method a bit confusing and would be glad to tidy it up. I just want to make sure I understand the copy_dotfolder() method correctly and to not break stuff:

• The first if condition should not occur, since the only dotfolders backed up are .vim, .ssh and possibly the sublime folders. Is this correct?
• The second if condition covers the default case wherein the dotfolder (.ssh, .vim) are backed up, isn't it?
• About the sublime folders (third if condition): this backs up the sublime packages and only yours, since your home directory is hard coded (I would refactor that also). Maybe in the long run, this back up should be moved to the package backup method, right?

I have now implemented all your ideas/change requests, besides the copy_dotfolder(), because I wanted to clarify my doubts first. I also want to implement this as soon as you confirm, however it may take a while until I find the time to sit down and write it.

BTW, I guess my IDE auto formatted the code and introduced the indentation issue, because I was actually careful to not add a different indentation style.

[alichtman]

(This got a lot longer than I was expecting, but I think being thorough is important.)

---

I find the copy_dotfolder() method a bit confusing and would be glad to tidy it up. I just want to make sure I understand the copy_dotfolder() method correctly and to not break stuff

Yep, let's walk through this method.

The first if condition should not occur, since the only dotfolders backed up are .vim, .ssh and possibly the sublime folders. Is this correct?

Right, this was for a feature to back up macOS Preferences .plist files that I decided to remove since you can't safely hot-swap files across macOS versions (setting names are neither forwards nor backwards compatible.) It's better to have a set-up script.

Let's remove that first condition and rename some vars, so it now looks like this:

def copy_dir(dir, backup_path):
	"""
	Copy directory from $HOME.
	"""
	invalid = {".Trash", ".npm", ".cache", ".rvm"}

	if len(invalid.intersection(set(dir.split("/")))) == 0:
		command = ""
		if "Application\ Support" not in dir:
			command = "cp -aRp " + dir + " " + backup_path + "/" + dotfolder.split("/")[-2]
		elif "Sublime" in dir:
			command = "cp -aRp " + dir + " " + backup_path + "/" + dotfolder.split("/")[-3]
		sp.run(command, shell=True, stdout=sp.PIPE)

The second if condition covers the default case wherein the dotfolder (.ssh, .vim) are backed up, isn't it?

To the best of my memory, yes. But there might be some other cases covered by the if "Application\ Support" not in dir: condition. I'd test this thoroughly before continuing with that assumption.

this backs up the sublime packages and only yours, since your home directory is hard coded (I would refactor that also).

That's my mistake. The path for my machine should definitely not be hardcoded in... Thanks for offering to fix that.

Here's the code block that will need to be modified (and also any other references to hardcoded paths.)

# Sublime Text Configs
	if os.path.isdir("/Users/alichtman/Library/Application Support/Sublime\ Text\ 2"):
		dotfolders_mp_in.append((os.path.join(home_path, "Library/Application\ Support/Sublime\ Text\ 2/Packages/User"), backup_path))

	if os.path.isdir("/Users/alichtman/Library/Application Support/Sublime\ Text\ 3"):
		dotfolders_mp_in.append((os.path.join(home_path, "Library/Application\ Support/Sublime\ Text\ 3/Packages/User"), backup_path))

Maybe in the long run, this back up should be moved to the package backup method, right?

We should back up settings files in the backup_config method, and Sublime/Atom/VSCode extensions in the backup_packages section. And have the reinstall method calls handle everything properly.

If you have suggestions on how to organize this better, I'm open to talking about them.

BTW, I guess my IDE auto formatted the code and introduced the indentation issue, because I was actually careful to not add a different indentation style.

I made exactly the same mistake the first time I contributed to an open source project. One of the best ways to learn is to make the mistake yourself :)

Let's also update the README to accurately reflect the functionality of the project.

---

I have now implemented all your ideas/change requests, besides the copy_dotfolder(), because I wanted to clarify my doubts first. I also want to implement this as soon as you confirm, however it may take a while until I find the time to sit down and write it.

Dope work so far, man. I understand the time restrictions, whenever you have it ready for another review, just let me know.

[alichtman]

Here's more of a thorough code review. Looks great so far, just some minor changes to make.

[raypatterson77]

because of my interest in issue #67 I want to ask if you are already working on extracting the hardcoded paths?
if you want I can pick this up.

[schilli91]

Hey,
I tried to fix all points from the review, although I had some comments.

About the copy_dotfolders method:
I think I get the copy_dotfolders method now. However, there is lots of logic in this method and the splitting of the dir. My _copy_dir_content(source, target) method uses a copy source to target logic, which the copy_dotfolders does not use. It calculates the target based on the source's values.
Therefore I find it rather hard to combine both functions. In my opinion, it would make sense to refactor the backup of dotfiles and their reinstalling, since the backup_dotfiles method seems to require the logic inside the copy_dotfolders method. Consequently, more code can be refactored and enhanced.

Would it be alright for you to keep both copy methods, since the copy_dotfolders clearly states its purpose of handling dotfolders, as well as _copy_dir_content clearly states it is a generic method to copy a dir's content?

@raypatterson77 I have not yet started the extraction, so you could start doing it, but if this PR is merged soon, it may be worth to do the issue #67 after that. Otherwise we might create some conflicts.

[alichtman]

I added this as a comment on the PR, but it's here for continuity in the conversation:

---

Both methods copy one directory to another path. We should be able to merge them together into one function that can serve all purposes (without complicating the logic too much further.)

I think all that would be needed is one extra else condition in the original copy_dir function.

[alichtman]

@raypatterson77 This PR is going to be merged soon (we're really close to finishing all this up.) So maybe the best move is to chill until this is merged and then start refactoring from there?

[alichtman]

These two comments are the last two things that jump out at me. I think all issues have been resolved, apart from these two one-line refactors.

I'm ready to merge this in after these are fixed (unless there's something I'm missing.)

Awesome work, @schilli91!

[alichtman]

I've decided to use autopep8 for formatting (and merged a PR to update master to reflect that), so you should format your PR the same way.

Sorry for making you undo all those spacing changes earlier. PEP8 formatting should be the standard, not whatever looks good on my IDE.

I reverted it after deciding I liked it more with tabs than spaces. idk, spacing standards are one of the less important standards, imo.

Also, make sure you put your info in CONTRIBUTORS.md!

[schilli91]

I had a really hard time merging after recent PRs. I hope everything is now resolved and we can merge. 😛

[alichtman]

This looks good to me.

Fix #59, #48

[alichtman]

Great work, again, @schilli91!

Thanks for contributing!