HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythongitModerate

Git command: push the latest commit and email the diff in colour

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
committhelatestemailpushgitdiffcolourandcommand

Problem

This is my first attempt a for git command I can use to push the latest commit and email the diff. Story behind how it came up: http://goo.gl/3NEHvn

Revision 1 (Scroll down for 2nd Revision)

```
import argparse
import subprocess
import os
import re
import send
import sys

class INVALIDEMAIL(Exception):
pass

def main():
parser = argparse.ArgumentParser(prog='ipush', description='Utility to push the last commit and email the color diff')
parser.add_argument('-v', '--verbose', action='store_true', help='if enabled will spit every command and its resulting data.')
parser.add_argument('-c', '--compose', help='text message to be sent with diff')
parser.add_argument('-to', type=str, help='enter a valid email you want to send to.')
parser.add_argument('-ver', '--version', action='version', version='%(prog)s 1.0')
parser.add_argument('-d', '--diff', required=False, help='if present pass arguments to it as you will do to git diff in inverted commas')
args = parser.parse_args()

VERBOSE = args.verbose

optArgs = vars(args)
if optArgs['to']:
sendTo = optArgs['to']
pattern = "^([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})$"
if not re.match(pattern, sendTo) != None:
raise INVALIDEMAIL("You have entered an invalid email to send to.")
else:
sendto = "removed@myemail.com"

diffCmd = 'git diff HEAD^ HEAD' if not optArgs['diff'] else "git diff %s" % optArgs['diff']

branchName, _ = execGitCommand('git rev-parse --abbrev-ref HEAD')

# stripping newline character which got appended when pulling branch name
branchName = branchName.split("\n")[0]
commitComment, _ = execGitCommand('git log -1 --pretty=%B')
subject = "%s: %s" % (branchName, commitComment)

# check for fatal error when executing git command
diffData, error = execGitCommand(diffCmd, VERBOSE)
if 'fatal' not in error.split(":"):
modifiedData, error = execGitCommand('git status', VERBOSE)
if

Solution

Here's my review on the Python code :

As a general comment, your code does not follow this. I'll try to hilight the different points as I see them but for the most obvious one, the namming convention is not respected. You can find tools to check automatically that everything is fine. For instance, http://pep8online.com/ .

This can take a default value as an argument. I guess you could use this to remove the if optArgs['to'] logic.

Comparisons to singletons like None should always be done with is or is not, never the equality operators.

if not re.match(pattern, sendTo) != None:


becomes

if re.match(pattern, sendTo) is  None:


You could remove duplication from

diffCmd = 'git diff HEAD^ HEAD' if not optArgs['diff'] else "git diff %s" % optArgs['diff']


by writing it

diffCmd = "git diff %s" % (optArgs['diff'] if optArgs['diff'] else "HEAD^ HEAD")


Purely personal but I find

message = htmlDiff if not optArgs['compose'] else "%s%s" % (optArgs['compose'], htmlDiff)


easier to read when conditions are not inverted:

message = "%s%s" % (optArgs['compose'], htmlDiff) if optArgs['compose'] else htmlDiff


I do not think eachLine is a very good variable name. line seems to be simple enough (and it does follow the naming conventions).

def getHtml(diffData):
    """ This method convertes git diff data to html color code
    """
    lines = ""
    openTag = ""
    nbsp = '    '
    for eachLine in diffData:
        if eachLine.startswith('-'):
            tabs = eachLine.count('\t')
            lines += "%s#ff0000%s%s%s" % (openTag, openTagEnd, nbsp*tabs ,eachLine)
        elif eachLine.startswith('+'):
            tabs = eachLine.count('\t')
            lines += "%s#007900%s%s%s" % (openTag, openTagEnd, nbsp*tabs, eachLine)
        else:
            tabs = eachLine.count('\t')
            lines += "%s#000000%s%s%s" % (openTag, openTagEnd, nbsp*tabs, eachLine)
    return lines


can be enhanced by removing duplicated code :

def getHtml(diffData):
    """ This method convertes git diff data to html color code
    """
    lines = ""
    openTag = ""
    nbsp = '    '
    for line in diffData:
        if line.startswith('-'):
            value = '#ff0000'
        elif line.startswith('+'):
            value = '#007900'
        else:
            value = '#000000'
        tabs = line.count('\t')
        lines += "%s%s%s%s%s" % (openTag, value, openTagEnd, nbsp*tabs ,line)
    return lines


Then, PEP 8 advices to use ''.join() instead of multiple concatenation which can become a bit expensive. Here's how you could do.

def getHtml(diffData):
    """ This method convertes git diff data to html color code
    """
    lines = []
    openTag = ""
    nbsp = '    '
    for line in diffData:
        if line.startswith('-'):
            value = '#ff0000'
        elif line.startswith('+'):
            value = '#007900'
        else:
            value = '#000000'
        tabs = line.count('\t')
        lines.append("%s%s%s%s%s" % (openTag, value, openTagEnd, nbsp*tabs ,line))
    return ''.join(lines)


Now, if we want to go further, one can notice that the pattern l=[]. for x in X: l.append(f(x)) looks like it could be some kind of list comprehension/generator expression.
Thus :

def getHtml(diffData):
    """ This method convertes git diff data to html color code
    """
    openTag = ""
    nbsp = '    '
    lines = []
    for line in diffData:
        value = '#ff0000' if line.startswith('-') else ('#007900' if line.startswith('+') else '#000000')
        tabs = line.count('\t')
        lines.append("%s%s%s%s%s" % (openTag, value, openTagEnd, nbsp*tabs ,line))
    return ''.join(lines)


becomes (but it might be a bit too excessive):

def getHtml(diffData):
    """ This method convertes git diff data to html color code
    """
    openTag = ""
    nbsp = '    '
    return ''.join([("%s%s%s%s%s" % (openTag, '#ff0000' if line.startswith('-') else ('#007900' if line.startswith('+') else '#000000'), openTagEnd, nbsp*line.count('\t') ,line)) for line in diffData])


You do not need to check if attachments: before doing for phile in attachments:.

That's pretty much all I have to say about the code itself for the time being.

Now for the functional side of it, I am not quite sure what was intended but you might want to have a look at git hooks as they would be a nice way to send the email automatically as you commit/push.

Code Snippets

if not re.match(pattern, sendTo) != None:
if re.match(pattern, sendTo) is  None:
diffCmd = 'git diff HEAD^ HEAD' if not optArgs['diff'] else "git diff %s" % optArgs['diff']
diffCmd = "git diff %s" % (optArgs['diff'] if optArgs['diff'] else "HEAD^ HEAD")
message = htmlDiff if not optArgs['compose'] else "%s<br><br>%s" % (optArgs['compose'], htmlDiff)

Context

StackExchange Code Review Q#40084, answer score: 10

Revisions (0)

No revisions yet.