1
\$\begingroup\$

The goal is pretty straightforward. There’s a list of dotfiles from my home directory. It loops through them and rsyncs all file to a Dropbox folder for backup purposes. A cron job runs this nightly. Code has worked well so far.

#!/usr/bin/python

import os
from os.path import expanduser
import sys
import subprocess

dotfiles = [
  '.ackrc',
  '.tmux.conf',
  '.vimrc',
  '.zshrc',
  '.gitconfig',
  '.ssh/config',
  '.zsh/MyAntigen.hs',
  '.vim/colors/dracula.vim'
]

home = expanduser("~")
src = home + '/'
dest = home + '/Dropbox/Dotfiles'
rsync = 'rsync --times'

if sys.stdout.isatty():
    rsync = rsync + ' --progress'

for item in dotfiles:
    src_file = os.path.join(src + item)
    cmd = rsync + ' ' + src_file + ' ' + dest
    process = subprocess.Popen(cmd, shell=True)
    process.wait()

This is my very first Python script (first time using rsync too) and I just started very recently. I’ve put this together based on trial and error and a lot of Stack Overflow. So I’m here for some genuine feedback.

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Not much to complain for your first script, congrats.

I'd perhaps change it so that all files are rsynced with the same command, if you look at the rsync documentation multiple sources may be specified for a single target. This would be more relevant for bigger programs - you usually want to avoid running multiple processes when you can have a single one do all the work, it makes bookkeeping easier in the long run (and it's faster). I'll leave that as an exercise for the reader though.

Since expanduser will perform tilde expansion I'd not split off home like this, just expand it twice, it's more readable IMO if you know what ~ means for paths (and most readers will know). If you do keep it like this, then consistently use os.path.join instead of concatenating / manually. Actually the one use of os.path.join in the script is wrong, it should be os.path.join(src, item).

The isatty is a nice touch.

For executing the process you're concatenating all arguments and then also run them through a shell - the concatenating is problematic when you have filenames with spaces in them, so in general I'd really rather go with the list argument approach. (Also see the security notes on the documentation page of subprocess.) If you're going through the shell there's also no need for the manual expanduser; you can easily test this if you prefix the rsync with echo (like I did to test the script).

Looking at the documentation Popen can of course be used like this, but it's not the most convenient option, I'd say either call or check_call are more appropriate here - both wait for the process to complete, while the latter also checks the exit status, which is vital for more complex programs (without it you don't know if the process completed successfully)!

#!/usr/bin/python

import os
from os.path import expanduser
import sys
import subprocess

dotfiles = [
    '.ackrc',
    '.tmux.conf',
    '.vimrc',
    '.zshrc',
    '.gitconfig',
    '.ssh/config',
    '.zsh/MyAntigen.hs',
    '.vim/colors/dracula.vim'
]

src = expanduser('~')
dest = expanduser('~/Dropbox/Dotfiles')

rsync = ['rsync', '--times']
if sys.stdout.isatty():
    rsync.append('--progress')

for item in dotfiles:
    subprocess.check_call(rsync + [os.path.join(src, item), dest])
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Went over the documentation you mentioned. Makes sense and was really helpful. Thanks! \$\endgroup\$ Aug 19, 2018 at 19:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.