Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am a newbie regarding Python and I would like to improve my Python-fu. As a learning exercise, I decided to write a script for helping me maintain my Unix configuration/dot-files under a git repository.

Before the code, some background on how it works: I check out a copy of the repository in my home directory called .dotfiles, and, inside that, I have the Python script called linkify, which deals with creating links from my home directory to the real files inside the .dotfiles directory.

Any comments about this code (and I do really mean any) is kindly welcome.

So, without further ado, here is the code (for your reference, the respective code is in my github repository):

New version after incorporation of suggestions

Just for the record, I have incorporated the changes received in the answers in my repository and I would love to receive further comments if that's not good. Of course, proper credit is given there for the changes suggested. Further comments are still appreciated, as I am a newbie with Python.

#!/usr/bin/env python

import errno
import os

to_keep_private = [
    'cifs-credentials',
    'config',
    'fetchmailrc',
    'gitconfig',
    'gnupg',
    'mpoprc',
    'msmtprc',
    'mutt',
    'netrc',
    'offlineimaprc',
    'pim',
    'purple',
    'quodlibet',
    'ssh',
    ]

to_ignore = [
    '.',
    '.git',
    '.gitignore',
    'linkify',
]


def make_private(files=[]):
    if len(files) == 0:
        files = os.listdir('.')
    else:
        # Use the list of files passed by the user, if it is not empty
        pass
    for filename in files:
        if os.path.isdir(filename):
            os.chmod(filename, 0700)
        else:
            os.chmod(filename, 0600)


def linkify(files=[]):
    if len(files) == 0:
        files = os.listdir('.')
    else:
        # Use the list of files passed by the user, if it is not empty
        pass

    os.chdir(os.path.join(os.environ['HOME'], '.dotfiles'))
    for source in files:
        if source not in to_ignore:
            target = os.path.join('..', '.' + source)
            source = os.path.join('.dotfiles', source)
            try:
                os.unlink(target)
            except OSError, e:
                if e.errno == errno.ENOENT:
                    pass
                elif e.errno == errno.EISDIR:
                    print("%s is a directory. Ignoring." % target)
                else:
                    print errno.errorcode[e.errno]
            finally:
                os.symlink(source, target)


def add_hook():
    f = open('.git/hooks/post-commit', 'w')
    f.write('#!/bin/sh\ngit push\n')
    f.close()
    os.chmod('.git/hooks/post-commit', 0755)


def parse_cli():
    import optparse
    parser = optparse.OptionParser()

    parser.add_option('-H',
                      '--install-hook',
                      help='Install git hook for automatic push.',
                      action='store_true',
                      default=False)
    parser.add_option('-n',
                      '--no-linkify',
                      help='Disable creation of symlinks',
                      action='store_true',
                      default=False)
    parser.add_option('-P',
                      '--no-fix-perms',
                      help='Disable fix of permission for private files.',
                      action='store_true',
                      default=False)

    return parser.parse_args()


if __name__ == '__main__':
    opts, args = parse_cli()

    if not opts.no_linkify:
        linkify(args)
    if not opts.no_fix_perms:
        make_private(args)
    if opts.install_hook:
        add_hook()
share|improve this question
add comment

2 Answers 2

up vote 4 down vote accepted

Some guidelines to follow:

  • If possible, as it is in your case, work with absolute path.
    You'll have at the beginning to do more or less something like:

    dir = '/home/rik/'
    files = [os.path.join(dir,file) for file in os.listdir(dir)]
    

    But everything should be easier and straighforward later on.

  • You need to move the default behaviour - files = os.listdir('.') - one level up.
    I can see one symptom and one reason for that:

    1. You've wrote two times the exact same code.
    2. The two different functions with that code should ignore what's your default directory, since they operate on list of files.

    So, if the user does not provide his own custom list, you need to generate the list of files in . and pass it to both your functions. This also means that the functions should not have a default argument value.

  • If you can, use argparse instead of optparse.
    I'm not sure from which version optparse was deprecated, but the main point is that if you have argpare in your standard library use that. You have only a couple of arguments so switching shouldn't take long.

  • Be consistent with your variable names.

    def make_private(files=[]):
        # [...]
        for filename in files:
            # [...]
    
    def linkify(files=[]):
        # [...]
        for source in files:
            # [...]
    

    Why one time files is a list of filenames and another a list of sources?
    If you strongly believe that one function works on filenames and the other on sources maybe you need to change the argument name, otherwise being consistent will help the readability of your code.

  • As Winston Ewert already told you, you should use capital letter for your constants names. Quoting from PEP8:

    Constants are usually defined on a module level and written in all capital letters with underscores separating words. Examples include MAX_OVERFLOW and TOTAL.

Additional notes

Don't do this:

def function(lst=[]):
    # ...

Do this instead:

def function(lst=None):
    if lst is None:
        lst = []
    # ... 

The reason for that is well explained here (or google it, because it's a very well known problem in which everyone stumbles sooner or later).

I'd say that by doing this you'll also remove the useless else, but as I explained it above you should remove the whole check and move it one level up against the user arguments.

share|improve this answer
    
Thanks for the comments. Both yours and @Winston's were very helpful and I chose yours as an answer because you have fewer reputation points (I hope that he doesn't care too much here). –  rbrito Mar 4 '12 at 9:28
    
thanks for the suggestions. They are very much appreciated. One thing that I didn't incorporate was the change to argparse from optparse, as I have systems with old versions of Python. (And, unfortunately, since I don't even have enough reputation points, I can't upvote your answer.) –  rbrito Mar 6 '12 at 2:13
add comment
#!/usr/bin/env python

import errno
import os

to_keep_private = [

By convention, global constants are in ALL_CAPS in python

    'cifs-credentials',
    'config',
    'fetchmailrc',
    'gitconfig',
    'gnupg',
    'mpoprc',
    'msmtprc',
    'mutt',
    'netrc',
    'offlineimaprc',
    'pim',
    'purple',
    'quodlibet',
    'ssh',
    ]

to_ignore = [
    '.',
    '.git',
    '.gitignore',
    'linkify',
]


def make_private(files=[]):

Generally, its dangerous to have mutable objects as defaults. They don't behave like we might expect.

    if len(files) == 0:
        files = os.listdir('.')

I'd expect an empty list of files to do nothing, not operate on the current directory. For this sort of case, its more usual to use None. Have None be the default and check for that.

    else:
        # Use the list of files passed by the user, if it is not empty
        pass

uhh.... this has no effect.

    for filename in files:
        if os.path.isdir(filename):
            os.chmod(filename, 0700)
        else:
            os.chmod(filename, 0600)


def linkify(files=[]):
    if len(files) == 0:
        files = os.listdir('.')
    else:
        # Use the list of files passed by the user, if it is not empty
        pass

Again. Don't do this.

    os.chdir(os.path.join(os.environ['HOME'], '.dotfiles'))

I avoid changing the current directory. It gives the function unintended semantics. For example all your os.listdir calls will return different results after this. Instead, use absolute paths.

    for source in files:
        if source not in to_ignore:
            target = os.path.join('..', '.' + source)
            source = os.path.join('.dotfiles', source)
            try:
                os.unlink(target)
            except OSError, e:
                if e.errno == errno.ENOENT:
                    pass
                elif e.errno == errno.EISDIR:
                    print("%s is a directory. Ignoring." % target)
                else:
                    print errno.errorcode[e.errno]

Do you really want to try carrying on after an unknown error code?

            finally:
                os.symlink(source, target)


def add_hook():
    f = open('.git/hooks/post-commit', 'w')
    f.write('#!/bin/sh\ngit push\n')
    f.close()

avoid one letter variable names. They make code harder to follow. Also you should use with

 with open('.git/hooks/post-commit', 'w') as hook:
     hook.write('#1/bin/sh\ngitush\n')

This will take of closing files even in the face of an exception.

    os.chmod('.git/hooks/post-commit', 0755)


def parse_cli():
    import optparse

Usually better to import everything at the top of the file, not in functions

    parser = optparse.OptionParser()

    parser.add_option('-H',
                      '--install-hook',
                      help='Install git hook for automatic push.',
                      action='store_true',
                      default=False)
    parser.add_option('-n',
                      '--no-linkify',
                      help='Disable creation of symlinks',
                      action='store_true',
                      default=False)
    parser.add_option('-P',
                      '--no-fix-perms',
                      help='Disable fix of permission for private files.',
                      action='store_true',
                      default=False)

    return parser.parse_args()

This is the bit where you actually interpreting the command line. I'd do the os.listdir() here as to my mind, its part of the command line interpretation.

if __name__ == '__main__':
    opts, args = parse_cli()

I'd call args: filenames to get a better idea of what's going on

    if not opts.no_linkify:
        linkify(args)
    if not opts.no_fix_perms:
        make_private(args)
    if opts.install_hook:
        add_hook()

Issues of else:

The original has the following code:

    if len(files) == 0:
        files = os.listdir('.')
    else:
        # Use the list of files passed by the user, if it is not empty
        pass

and I said this was a bad idea because the else did nothing. You've asked for clarification on this point. I think you put the else block in there to make it more explicit what you are doing, that way you have clearly delinated the two cases. I'd say if you want it to be explicit, make it explicit via code not comments

if len(files_) == 0:
    files = os.listdir(.)
else:
    files = files_

I think this is better because the code conveys the message, rather then inline english text.

However, I'm not sure that much is gained for explicitness here. If I see:

if len(files) == 0:
   files == os.listdir('.')

I can see that its implementing the logic for a default parameter, because that's a pretty common practice. I don't see that you gain much by making it more explict what the other case is. The other case is pretty obvious.

share|improve this answer
    
Thanks for comments @Winston. There are some points where I would appreciate some clarification, though., but there are some points where I would appreciate clarification. The most important of these are your comments regarding the else with a pass, even if it has a comment to not let the reader know that that's deliberate. –  rbrito Mar 4 '12 at 4:47
    
@rbrito, I've added to the answer trying to clarify a bit about my else/pass comments –  Winston Ewert Mar 5 '12 at 21:02
    
thanks for the additional comments regarding the else/pass issue. This is just a long-time habit that I carried over, as I (still) feel bad whenever I see an if without the corresponding else. BTW, I have implemented most of the issues you both pointed out in a new version of the code. Not everything, but almost all points. –  rbrito Mar 6 '12 at 2:06
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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