Merge lp:~knitzsche/dotdepends/addresses-mfisch-review into lp:dotdepends

[Kyle Nitzsche]

addresses most of mfisch's review comments. others (unit tests) left for carif.
    following bugs opened:
    - LP: #1023578. This is fixed in dotdepends-merge but should be addressed
      in dotdepends. User can now set node fill colors (and shapes) with args.
    - LP: #1023581. Not fixed.
    - LP: #1023564. Not fixxed.

[Matt Fischer (mfisch)]

changelog:
You misspelled "fixed" - LP: #1023564. Not fixxed. I think maybe you just want to not list bugs that you didn't fix yet though, it will be confusing I think.

copyright:
Sorry to mention this again, I think it's supposed to be, I think I just said "Canonical" before
Copyright: 2012 Canonical Ltd.

I have more questions about colors that I will follow-up with you in IRC about.

review: Needs Fixing

[lp:~knitzsche/dotdepends/addresses-mfisch-review]

74. By Kyle Nitzsche on 2012-07-11

addresses mfisch's review comments (except for those carif will do).

75. By Kyle Nitzsche on 2012-07-11

addresses non-unit test review comments

[Matt Fischer (mfisch)]

review: Approve

=== modified file 'Dotdepends/Dot.py'
--- Dotdepends/Dot.py	2012-07-10 21:54:13 +0000
+++ Dotdepends/Dot.py	2012-07-11 21:15:25 +0000
@@ -21,7 +21,9 @@
 import subprocess
 
 def dot2png(_f, shape="box"):
-    """Run dot on passed file handle to create png with box shapes as default"""
+    """Run dot on passed file handle to create png with box shapes as
+    default"""
+
     _fpng = _f.rstrip(".dot") + ".png"
     cmd = ['dot', '-Tpng', '-Nshape=%s' % shape, '-o', _fpng, _f]
     subprocess.call(cmd)
@@ -113,7 +115,7 @@
         """
 
         joins = []
-            # joins structure: [lnode:[rnode,style]]
+        # joins structure: [lnode:[rnode,style]]
         for line in self.content:
             line = line.strip()
             if len(line) == 0: continue
@@ -124,7 +126,11 @@
                 if len(tokens) == 3:
                     joins.append([tokens[0],tokens[2].rstrip(";\n"), ""])
                 if len(tokens) == 4:
-                    joins.append([tokens[0],tokens[2], tokens[3].rstrip(";\n")])
+                    joins.append(
+                        [tokens[0],
+                        tokens[2],
+                        tokens[3].rstrip(";\n")]
+                                )
         return joins
 
     def get_nodes(self):
@@ -467,15 +473,15 @@
                       type="string",
                       dest="root",
                       default="",
-                      help="The root node. Pass it to prevent summarizing the\
- root node into a family."
+                      help="The root node. Pass it to prevent summarizing the"\
+                        " root node into a family."
                      )
     parser.add_option("-o", "--out-file",
                       type="string",
                       dest="out_file",
                       default="out",
-                      help="The generated  dot filename without the '.dot'\
- extension."
+                      help="The generated  dot filename without the '.dot'"\
+                        " extension."
                      )
     parser.add_option("-d", "--dot-file",
                       type="string",
@@ -487,37 +493,38 @@
                       dest="summarize",
                       action="store_true",
                       default=False,
-                      help="Produce dot with families summarized as single\
- nodes"
+                      help="Produce dot with families summarized as single "\
+                        "nodes"
                      )
     parser.add_option("-l", "--get-popular-lnodes",
                       dest="get_pop_lnodes",
                       action="store_true",
                       default=False,
-                      help="Output lnodes that point to at least 'popularity'\
- rnodes"
+                      help="Output lnodes that point to at least 'popularity'"\
+                        " rnodes"
                      )
     parser.add_option("-c", "--get-popular-rnodes",
                       dest="get_pop_rnodes",
                       action="store_true",
                       default=False,
-                      help="Output rnodes that point to at least 'popularity'\
- lnodes"
+                      help="Output rnodes that point to at least 'popularity'"\
+                        " lnodes"
                      )
     parser.add_option("-p", "--popularity",
                       type="string",
                       dest="popularity",
                       default="",
-                      help="The minimum poplarity needed to be reported when -l\
- or -r are used."
+                      help="The minimum poplarity needed to be reported when"\
+                        " -l or -r are used."
                      )
     parser.add_option("-n", "--dont-run-dot",
                       dest="dont_run_dot",
                       action="store_true",
                       default=False,
-                      help="Execute dot to process the dot file to a png. Note\
- very large dot files produced in rdepends mode for packages with many rdepends\
- may take an unreasonable amount of time for dot to process."
+                      help="Execute dot to process the dot file to a png. "\
+                        "Note very large dot files produced in rdepends mode"\
+                        " for packages with many rdepends may take an "\
+                        "unreasonable amount of time for dot to process."
                     )
     parser.add_option("-N", "--node-shape",
                       type="string",

=== modified file 'debian/changelog'
--- debian/changelog	2012-07-11 17:30:35 +0000
+++ debian/changelog	2012-07-11 21:15:25 +0000
@@ -1,3 +1,9 @@
+dotdepends (0.3.16kyle1) UNRELEASED; urgency=low
+
+  * addresses non-unit test review comments
+
+ -- Kyle Nitzsche <[e-mail]>  Wed, 11 Jul 2012 17:12:39 -0400
+
 dotdepends (0.3.16) precise; urgency=low
 
   * redundant 'color="red"' styles in nodes in generated pkg-DETAIL.dot files

=== modified file 'debian/copyright'
--- debian/copyright	2012-07-11 18:18:21 +0000
+++ debian/copyright	2012-07-11 21:15:25 +0000
@@ -3,7 +3,7 @@
 Upstream-Contact: Kyle Nitzsche <[e-mail]>
 
 Files: *
-Copyright: (C) 2012 Kyle Nitzsche, Canonical
+Copyright: (C) Canonical Ltd. 2012
 License: GPL-3
  The full text of the GPL is distributed in
  /usr/share/common-licenses/GPL-3 on Debian systems.

=== modified file 'debian/rules'
--- debian/rules	2012-07-10 21:54:13 +0000
+++ debian/rules	2012-07-11 21:15:25 +0000
@@ -4,7 +4,8 @@
 	dh $@ --with python2
 
 override_dh_auto_install:
-	python ./setup.py install --root=$(CURDIR)/debian/tmp --prefix=/usr --install-layout=deb
+	python ./setup.py install --root=$(CURDIR)/debian/tmp --prefix=/usr\
+ --install-layout=deb
 
  override_dh_auto_clean:
 	-find . -name '*.py[co]' | xargs rm -f

=== modified file 'usr/bin/dotdepends'
--- usr/bin/dotdepends	2012-07-10 21:54:13 +0000
+++ usr/bin/dotdepends	2012-07-11 21:15:25 +0000
@@ -25,7 +25,8 @@
 
 # Running dotdepends "in place" in the source pkg
 # Add location of modules to path
-sys.path.insert(0, path.abspath(path.dirname(path.abspath(__file__)) + '../../..'))
+sys.path.insert(0, path.abspath(path.dirname(path.abspath(__file__)) + \
+'../../..'))
 # Add location of executable to path because it calls itself
 location = path.abspath(path.dirname(path.abspath(__file__)))
 sys.path.insert(0, location)
@@ -138,7 +139,11 @@
 if __name__ == "__main__":
 
     # dotdepends with no arguments or with -h should run help
-    if len(sys.argv) == 1 or sys.argv[1] == '-h' or sys.argv[1].find('help') > -1:
+    if (
+        len(sys.argv) == 1 
+        or sys.argv[1] == '-h' 
+        or sys.argv[1].find('help')
+        ) > -1:
         print sys.stderr, "See 'man dotdepends' for help"
         sys.exit(0)
 
@@ -191,15 +196,16 @@
     pkg =""
     #print args
     if len(args) != 1:
-        print sys.stderr, "Error. A package must be be provided as the root node for the\
-               graph. Use '-h'. Stopping"
+        print sys.stderr, "Error. A package must be be provided as the root "\
+            "node for the graph. Use '-h'. Stopping"
     else:
         pkg = args[0]
 
 
     # User set -m depends explicitly.
     if opt.mode == 'depends' and opt.installed:
-        print >> sys.stderr, "Error. Installed (-i) is not valid for mode 'depends'. Stopping."
+        print >> sys.stderr, "Error. Installed (-i) is not valid for mode "\
+            "'depends'. Stopping."
         sys.exit(1)
 
     #print "pkg %s" % pkg
@@ -221,13 +227,14 @@
 
 
         if opt.installed:
-            print >> sys.stderr, "Error. Installed (-i) is not valid in default ('analyze') mode. Stopping."
+            print >> sys.stderr, "Error. Installed (-i) is not valid in "\
+                "default ('analyze') mode. Stopping."
             sys.exit(1)
 
 
         if opt.mode == "rdepends":
-            print sys.stderr, "Error. Analyze (-a) is only applicable to 'depends' mode\
-                (not '-m redepends'). Stopping."
+            print sys.stderr, "Error. Analyze (-a) is only applicable to "\
+                "'depends' mode (not '-m redepends'). Stopping."
             sys.exit(1)
 
         cmd = [
@@ -240,16 +247,22 @@
         if opt.dont_run_dot:
             cmd.append('-d')
 
-        # output_dir_created is True iff the subprocess below will create (and populate)
-        #   a new directory opt.output_dir.
-        # If subprocess creates the directory, but then we error out, we should clean up the directory.
+        # output_dir_created is True iff the subprocess below will create (and 
+        # populate) a new directory opt.output_dir.
+        # If subprocess creates the directory, but then we error out, we should
+        # clean up the directory.
         # We should not, however, remove directories we don't create.
         output_dir_created = not path.isdir(opt.output_dir)
 
-        p = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+        p = subprocess.Popen(
+                        cmd,
+                        stdout=subprocess.PIPE,
+                        stderr=subprocess.PIPE
+                        )
         f1, error = p.communicate()
         if p.returncode != 0 or error.startswith('E:'):
-            print >> sys.stderr, "Error. '%s'. Stopping." % error.rstrip() # remove possible newlines
+            print >> sys.stderr, "Error. '%s'. Stopping." % error.rstrip()
+            # ^ remove possible newlines
             if output_dir_created: shutil.rmtree(opt.output_dir)
             # return process return code iff more specific
             sys.exit(max(1, p.returncode))
@@ -269,7 +282,10 @@
         p = subprocess.Popen(cmd, stdout=subprocess.PIPE)
         f2 = p.communicate()
         # Did the process error?
-        if p.returncode > 0 or (f2[1] is not None and f2[1].startswith('Error')):
+        if p.returncode > 0 or (
+                            f2[1] is not None
+                            and f2[1].startswith('Error')
+                                ):
             # Yes, pass the error along
             print >> sys.stderr, f2[1]
             sys.exit(1)

=== modified file 'usr/bin/dotdepends-merge'
--- usr/bin/dotdepends-merge	2012-07-10 21:54:13 +0000
+++ usr/bin/dotdepends-merge	2012-07-11 21:15:25 +0000
@@ -17,48 +17,15 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 import sys
+from optparse import OptionParser
 from Dotdepends.Dot import Dot as Dot, replacement
 
-if "-h" in sys.argv or "--help" in sys.argv:
-    print "Use 'man dotdepends-merge' to display help."
-    sys.exit(1)
-
-# dot files
-f1 = sys.argv[1]
-f2 = sys.argv[2]
-# shapes for nodes
-s1 = "box"
-s2 = "ellipse"
-s3 = "diamond"
-# colors for nodes
-c1 = "grey"
-c2 = "brown"
-c3 = "orange"
-# load dot files from API
-d1 = Dot(f1)
-d2 = Dot(f2)
-# get root pkgs for each
-r1 = d1.root
-r2 = d2.root
-
-# quit if no root
 def verify_root(d):
+    """quit if no root pkg defined in dot file"""
+
     if len(d.root) < 1:
         print sys.stderr("Error. %s has no defined root pkg. Stopping") % d
         sys.exit(1)
-verify_root(d1)
-verify_root(d2)
-
-n1 = d1.get_nodes()
-n1[replacement(r1)] = 'label="%s"' % r1
-j1 = d1.get_joins()
-n2 = d2.get_nodes()
-n2[replacement(r2)] = 'label="%s"' % r2
-j2 = d2.get_joins()
-
-_n1 = dict()
-_n2 = dict()
-rootNodeLines = []
 
 def addShapes(nodes, shape, color):
     newnodes = dict()
@@ -76,120 +43,200 @@
         _styles['shape'] = shape
         newnodes[n] = _styles
     return newnodes
-_n1 = addShapes(n1,s1,c1)
-_n2 = addShapes(n2,s2,c2)
-
-_shape='shape="diamond",fillcolor="orange",style="filled"'
-_nFinal = dict()
-
-nBoth = set(_n1.keys()).intersection(set(_n2.keys()))
-n1Only = set(_n1.keys()).difference(set(_n2.keys()))
-n2Only = set(_n2.keys()).difference(set(_n1.keys()))
-
-nWorking = dict()
-nFinal = dict()
-
-for node in nBoth:
-    n = _n1[node]
-    # if color:red is in both node sets, this pkg is only a Rec
-    # and needs to be red here too.
-    aRec = False
-    try:
-        if 'color' in _n1[node].keys():
-            if _n1[node][color] == 'red':
-                aRec = True
-    except:
-        pass
-    try:
-        if 'color' in _n2[node].keys():
-            if _n2[node][color] == 'red':
-                aRec = True
-            else:
-                aRec = False
-    except:
-        aRec = False
-
-    if aRec:
-        n['color'] = 'red'
-
-    n['shape'] = s3
-    n['fillcolor'] = c3
-    n['style'] = 'filled'
-    nFinal[node] = n
-
-for node in n1Only:
-    n = _n1[node]
-    aRec = False
-    try:
-        if 'color' in _n1[node].keys():
-            if _n1[node][color] == 'red':
-                n['color'] = 'red'
-    except:
-        pass
-    n['shape'] = s1
-    n['fillcolor'] = c1
-    n['style'] = 'filled'
-    nFinal[node] = n
-
-for node in n2Only:
-    n = _n2[node]
-    try:
-        if 'color' in _n1[node].keys():
-            if _n1[node][color] == 'red':
-                n['color'] = 'red'
-    except:
-        pass
-    n['shape'] = s2
-    n['fillcolor'] = c2
-    n['style'] = 'filled'
-    nFinal[node] = n
-
-for node in nFinal.keys():
-# set fillcolor of root nodes blue
-    n = dict(nFinal[node])
-    pkg = node
-    if 'label' in n.keys():
-        pkg = n['label']
-    if pkg == r1 or pkg == r2:
-        n['fillcolor'] = 'blue'
-        n['fontcolor'] = 'white'
-    nFinal[node] = n
-
-# insert initial commented lines that show the root nodes
-for line in rootNodeLines:
-    print line
-
-# start the dot file content
-print "strict digraph dotdepends {"
-print "subgraph cluster_program {"
-
-# print the nodes lines
-for n in nFinal:
-    line = "%s [" % n
-    for style in nFinal[n]:
-        if style == '': continue
-        line += '%s="%s",' % (style, nFinal[n][style])
-    line = line[:len(line)-1]
-    line += '];'
-    print line
-
-#print the join ('edge') lines
-def printJoins(j):
-    for join in j:
-        #print "JOIN %s" % join
-        line = "%s -> %s" % (join[0], join[1])
-        if len(join[2]) > 0:
-            line += " %s" % join[2]
-        line += ';'
-        print line
-
-printJoins(j1)
-printJoins(j2)
-
-# finish the dot file
-print "}"
-print "}"
-
-sys.exit(0)
+
+if __name__ == "__main__":
+
+    # dotdepends with no arguments or with -h should run help
+    if (
+        len(sys.argv) == 1
+        or sys.argv[1] == '-h'
+        or sys.argv[1].find('help')
+        ) > -1:
+        print sys.stderr, "See 'man dotdepends' for help"
+        sys.exit(0)
+
+    parser = OptionParser()
+
+    # NOTE: command help is in the dotdepends man page
+
+    parser.add_option("-c", "--color-1",
+                      dest="color1",
+                      type="string",
+                      default="grey",
+                     )
+    parser.add_option("-d", "--color-2",
+                      dest="color2",
+                      type="string",
+                      default="brown",
+                     )
+    parser.add_option("-e", "--color-3",
+                      dest="color3",
+                      type="string",
+                      default="orange",
+                     )
+    parser.add_option("-s", "--shape-1",
+                      dest="shape1",
+                      type="string",
+                      default="box",
+                     )
+    parser.add_option("-t", "--shape-2",
+                      dest="shape2",
+                      type="string",
+                      default="ellipse",
+                     )
+    parser.add_option("-u", "--shape-3",
+                      dest="shape3",
+                      type="string",
+                      default="diamond",
+                     )
+
+    (opt, args) = parser.parse_args()
+
+    # dot files
+    f1 = sys.argv[1]
+    f2 = sys.argv[2]
+    # shapes for nodes
+    shape1 = opt.shape1
+    shape2 = opt.shape2
+    shape3 = opt.shape3
+    # colors for nodes
+    color1 = opt.color1
+    color2 = opt.color2
+    color3 = opt.color3
+    # load dot files from API
+    d1 = Dot(f1)
+    d2 = Dot(f2)
+    # get root pkgs for each
+    r1 = d1.root
+    r2 = d2.root
+
+    verify_root(d1)
+    verify_root(d2)
+
+    n1 = d1.get_nodes()
+    n1[replacement(r1)] = 'label="%s"' % r1
+    j1 = d1.get_joins()
+    n2 = d2.get_nodes()
+    n2[replacement(r2)] = 'label="%s"' % r2
+    j2 = d2.get_joins()
+
+    _n1 = dict()
+    _n2 = dict()
+    rootNodeLines = []
+    _n1 = addShapes(n1,shape1,color1)
+    _n2 = addShapes(n2,shape2,color2)
+
+    _shape='shape="diamond",fillcolor="orange",style="filled"'
+    _nFinal = dict()
+
+    nBoth = set(_n1.keys()).intersection(set(_n2.keys()))
+    n1Only = set(_n1.keys()).difference(set(_n2.keys()))
+    n2Only = set(_n2.keys()).difference(set(_n1.keys()))
+
+    nWorking = dict()
+    nFinal = dict()
+
+    for node in nBoth:
+        n = _n1[node]
+        # if color:red is in both node sets, this pkg is only a Rec
+        # and needs to be red here too.
+        aRec = False
+        try:
+            if 'color' in _n1[node].keys():
+                if _n1[node][color] == 'red':
+                    aRec = True
+        except:
+            pass
+        try:
+            if 'color' in _n2[node].keys():
+                if _n2[node][color] == 'red':
+                    aRec = True
+                else:
+                    aRec = False
+        except:
+            aRec = False
+
+        if aRec:
+            n['color'] = 'red'
+
+        n['shape'] = shape3
+        n['fillcolor'] = color3
+        n['style'] = 'filled'
+        nFinal[node] = n
+
+    for node in n1Only:
+        n = _n1[node]
+        aRec = False
+        try:
+            if 'color' in _n1[node].keys():
+                if _n1[node][color] == 'red':
+                    n['color'] = 'red'
+        except:
+            pass
+        n['shape'] = shape1
+        n['fillcolor'] = color1
+        n['style'] = 'filled'
+        nFinal[node] = n
+
+    for node in n2Only:
+        n = _n2[node]
+        try:
+            if 'color' in _n1[node].keys():
+                if _n1[node][color] == 'red':
+                    n['color'] = 'red'
+        except:
+            pass
+        n['shape'] = shape2
+        n['fillcolor'] = color2
+        n['style'] = 'filled'
+        nFinal[node] = n
+
+    for node in nFinal.keys():
+    # set fillcolor of root nodes blue
+        n = dict(nFinal[node])
+        pkg = node
+        if 'label' in n.keys():
+            pkg = n['label']
+        if pkg == r1 or pkg == r2:
+            n['fillcolor'] = 'blue'
+            n['fontcolor'] = 'white'
+        nFinal[node] = n
+
+    # insert initial commented lines that show the root nodes
+    for line in rootNodeLines:
+        print line
+
+    # start the dot file content
+    print "strict digraph dotdepends {"
+    print "subgraph cluster_program {"
+
+    # print the nodes lines
+    for n in nFinal:
+        line = "%s [" % n
+        for style in nFinal[n]:
+            if style == '': continue
+            line += '%s="%s",' % (style, nFinal[n][style])
+        line = line[:len(line)-1]
+        line += '];'
+        print line
+
+    #print the join ('edge') lines
+    def printJoins(j):
+        for join in j:
+            #print "JOIN %s" % join
+            line = "%s -> %s" % (join[0], join[1])
+            if len(join[2]) > 0:
+                line += " %s" % join[2]
+            line += ';'
+            print line
+
+    printJoins(j1)
+    printJoins(j2)
+
+    # finish the dot file
+    print "}"
+    print "}"
+
+    sys.exit(0)