patternpythongitModerate
Git command: push the latest commit and email the diff in colour
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
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
Comparisons to singletons like None should always be done with is or is not, never the equality operators.
becomes
You could remove duplication from
by writing it
Purely personal but I find
easier to read when conditions are not inverted:
I do not think
can be enhanced by removing duplicated code :
Then, PEP 8 advices to use
Now, if we want to go further, one can notice that the pattern
Thus :
becomes (but it might be a bit too excessive):
You do not need to check
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.
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 htmlDiffI 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 linescan 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 linesThen, 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.