current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 3 down vote favorite

I have created a small script that aims on comparing two versions of a file by doing a git diff. Is there a better way to code it?

import subprocess

def diff_versions(file_to_be_compared, directory):
    try:
        out = subprocess.check_output(["git", "diff", "origin/master^", "origin/master", file_to_be_compared],
            cwd = directory)
    except subprocess.CalledProcessError as ex:
        return ex.output

    return out is None

def main():
    result = diff_versions('file.txt', "some/directory")
    if result is False:
        raise Exception('diff found between the files')
    else:
        return 'files are identical'

if __name__ == "__main__":
    main()
share|improve this question
    
python has git libraries, those would almost certainly be better to use than calling git directly. – Etan Reisner Feb 4 '15 at 19:16
up vote 3 down vote 
if result is False:
    raise Exception('diff found between the files')
else:
    return 'files are identical'

Why would a difference be considered Exceptional? Given that nothing else happens in the program, there isn't much difference at this point between raising an error and just printing the message, but it seems like an odd way to do things.

Also, you shouldn't test for False by identity, if not result is the usual method.

Have you considered allowing the user to select branches to compare? You could use the current value as a default, e.g. 

 def diff_versions(file_to_be_compared, directory, branch="origin/master"):
share|improve this answer
    
Actually as this script is meant to run on a CD pipeline I might just do "assert result" or something similar instead of raising an Exception(). – Orestis Feb 4 '15 at 17:01
1  
That isn't a best-practice use of assert, see e.g. stackoverflow.com/q/944592/3001761 – jonrsharpe Feb 4 '15 at 17:05

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.