3
\$\begingroup\$

I am writing a script to automate the install and configuration of vim, zsh, ohmyzsh and tmux, on a Linux/Mac. I'd like to perform all operations in Python, rather than using subprocess to streamline some of the operations (e.g. git clone).

I would also like to use this script as an example to myself of good Python coding practice for file system operations. Code clarity, reuse and pythonic style are my primary objectives. Before I write the remainder of the script I'd like some input on the methods I've chosen for low level operations, but all areas of the script are open for scrutiny.

from urllib.request import urlretrieve
from urllib.error import URLError
from zipfile import ZipFile
import sys
import os
import errno
import shutil


vim = ".vimtest"
vim_directories = [vim, (vim + "/bundle"), (vim + "/autoload"), (vim + "/colors")]
vim_files = [".vimrctest", vim + "/vimrc"]
flags = os.O_CREAT | os.O_EXCL | os.O_WRONLY


def vim_install():

    # Create directories and files

    for sub in vim_directories:
        try:
            os.mkdir(os.path.expanduser('~/' + sub))
            print("mkkdir " + os.path.expanduser('~/' + sub))
        except OSError as e:
            if e.errno == errno.EEXIST:
                print("Directory " + vim + sub + " already exists.")
                sys.exit(1)
            else:
                raise

    try:
        file_handle = os.open(os.path.expanduser('~/' + vim_files[1]), flags)
    except OSError as e:
        if e.errno == errno.EEXIST:
            print("File " + vim_files[1] + " already exists")
            sys.exit(1)
        else:
            raise
    with os.fdopen(file_handle, 'w') as file_obj:
        file_obj.write("")
    print("Created file " + os.path.expanduser('~/') + vim_files[1])

    try:
        os.symlink(os.path.expanduser('~/') + vim_files[1], 
        os.path.expanduser('~/') + vim_files[0])
        print("ln -s " + os.path.expanduser('~/') + vim_files[0] + " -> " +
        os.path.expanduser('~/') + vim_files[1])

    except OSError:
        print("Failed to make symbolic link.")
        print("ln -s " + os.path.expanduser('~/') + 
        vim_files[0] + " -> " + os.path.expanduser('~/') +
        vim_files[1])
        sys.exit(1)

    # Download Pathogen, NerdTree, unpack

    try:
        urlretrieve("https://tpo.pe/pathogen.vim",
                        os.path.expanduser('~/' + vim + "/autoload/pathogen.vim"))
        print("Downloaded Pathogen")

        urlretrieve("https://github.com/scrooloose/nerdtree/archive/master.zip",
                        os.path.expanduser('~/' + vim + "/bundle/master.zip"))
        print("Downloaded NerdTree")

    except URLError as e:
        if hasattr(e, 'reason'):
            print('Scrip failed to reach a server.')
            print('Reason: ', e.reason)
        elif hasattr(e, 'code'):
            print('The server couldn\'t fulfill the request.')
            print('Error code: ', e.code)
       else:
            raise

    try:
        zip_ref = ZipFile(os.path.expanduser('~/') + vim + 
        "/bundle/master.zip", 'r')
        zip_ref.extractall(os.path.expanduser('~/') + vim + "/bundle/")
        zip_ref.close()
        print("Unziped NerdTree")

    except OSError as e:
        print("Could not unzip: {0}".format(e))

    try:
        os.rename(os.path.expanduser('~/' + vim + "/bundle/nerdtree-master"),
        os.path.expanduser('~/' + vim + "/bundle/nerdtree"))
        print("Renamed " + os.path.expanduser('~/') + vim + 
        "/bundle/nerdtree-master \n"
          + os.path.expanduser('~/') + vim + "/bundle/nerdtree")

    except OSError as e:
        print("OS error {0}".format(e))
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Of the software packages mentioned in the title, I only see Vim being installed by the code. \$\endgroup\$ May 26, 2018 at 5:38
  • \$\begingroup\$ I edited the title according to what your function actually does \$\endgroup\$ May 26, 2018 at 6:39

1 Answer 1

1
\$\begingroup\$
  • vim, vim_directories, vim_files and flags seem to be constants, so you should apply the naming conventions on them by capitalizing their names.

Reviewing the very first part of your function code

About the serious things, I would talk for the moment only about the very first part of your code, later I may come back (no promise though):

  • Within the try block, you should test only the code that is actually prone to raising an exception, nothing else. If you apply this rule, you will end up by coding short try blocks.
  • What may raise an exception here is rather the attempt to create a directory: os.mkdir(os.path.expanduser('~/' + sub)), the print() would not execute anyway if the attempt fails. If the directory creation succeeds, the else block should take care of the printing task. The try block should be concise and clean
  • I do not see the necessity of sys.exit(1) here. To be honest, in most cases it is useless, and in your typical case, I do not see how it could be useful, and you already print a message about execution failure.
  • As a reader of your code, I was curious to know why are you particularly interested errno.EEXIST but not other error codes? I was expecting that you are too meticulous and you decided to track down all the symbols of errno module, but I was mistaken. I think it is fair enough to write this instead:

    except OSError as e:
         print('Error code: {}'.format(e.errno))
         print('Error message: {}'.format(e.strerror))
    
  • Besides, note that this code:

    e.errno == errno.EEXIST:
        print('...')
    

    is the same as:

    e.errno == 17:
        print('...')
    

    What I mean here, do you really want to include such fine grain details in a function which is rather more generic/high level?

  • If for some mysterious reason of your own, you find errno.EEXIST too sexy and attractive, I would still opt for a less brain nagging syntax: FileExistsError instead of errno.EEXIST (note that the later corresponds exactly to FileExistsError but, as you can see, it is more readable)
  • Given the elements mentioned so far, I would write the very first part of your code as follows:

    for sub in vim_directories:
        try:
            os.mkdir(os.path.expanduser('~/' + sub)) 
        except FileExistsError:
            print("Directory " + vim + sub + " already exists.")         
        except OSError as e:
            print('Error code: {}'.format(e.errno))
            print('Error message: {}'.format(e.strerror))
        else:
            print("mkkdir " + os.path.expanduser('~/' + sub))
    
\$\endgroup\$
2
  • \$\begingroup\$ errno.EEXIST does not necessarily correspond to 17. These values are specific to your platform. The errno module is there to make the codes cross-compatible. \$\endgroup\$
    – Daniel
    May 26, 2018 at 9:36
  • \$\begingroup\$ @Billal, thanks for your input and feedback. I will take your advice about minimizing my try blocks to include only include statements that may generate exceptions. In catching errno.EEXIST and raising other exceptions, I was anticipating the broader structure of my final scripts, where I would write undo routines rather than print error statements. You've given me a few things to think about, thanks again for your advice. \$\endgroup\$ May 27, 2018 at 1:27

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.