-
Notifications
You must be signed in to change notification settings - Fork 26
Follow-up fixes for #112 #116
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
base: master
Are you sure you want to change the base?
Follow-up fixes for #112 #116
Conversation
|
Hi, I'm just letting you know that I'm busy right now so I haven't had time to go and review PRs for merging. |
|
I've read your code but half of it doesn't make sense. Let me comment on the actual code. |
You didn't actually do this and I think it also wouldn't be possible to do this and still make the text colored? This is heavily dependent on what rust allows to be in a const context. So I removed it while basing my previous fix on yours. |
I'm sorry for not being more clear - items #1 and #2 on my list (at the top of this thread) was me testing your code commit for #112, as per your request to "Test it out and let me know if something comes up". Your code does print out the entire message as the "long_about" for the from_stow_cmd (which is great!) and also prints out the message, with the yellow color, when the user runs the from_stow_cmd. So yeah - this was me trying to say that your code is awesome! 😁👍 |
I saw your commit for #112. It looks good and works well! There's a couple small things that I'm submitting this PR for.
I checked (and confirmed to be working):
1: from-stow wanted to convert the stow directory into a temporary directory but it failed because it never created the temp dir ; now it does
2: refactor help text so that it shows up when you run tuckr -h from-stow (not just when you run the command itself)
Minor errors:
3: if dotfiles_old already exists we exit with a clear message saying this (instead of an error when we try to rename the existing dotfiles dir)
The code is checking if the dotfiles directory (not dotfiles_old) exists and stopping at that point. My PR fixes this so that it'll print an error only when the _old directory exists. This means that if the dotfiles dir is there but not the _old dir then from-stow will move the current dotfiles dir into the backup _old dir
4: misc, minor adjustments to dry-run - tidying up messages, and also prevent a failure when the new, temp dotfiles dir wasn't created
i didn't verify this works by running it, but looking at the code there needs to be an 'else' for
if file.is_dir() {so that we don't try to create the dir in dry-run mode.New items:
5: If something goes wrong with the fs::copy(&file, &tuckr_path) operation the whole thing will halt immediately - it would be nice for this to print an error message and then keep going. This is probably a corner case, but I encountered it when I had a broken link in my stow-files dir. The real error is that I had a link in my stow-files dir, but having a nice error message would help folks figure that out.
6: This is super-minor, but the code always tell the user that the old dotfiles are available in dotfiles_old, even if we didn't create an _old directory.
One last request: could you accept my PR into the project? I'd really like to be able to brag about "I made a commit".