Code review 2

Author: einarf

pywavefront/obj.py

@@ -62,6 +62,8 @@ def parse_vn(self):
 
     def consume_normals(self):
         """Consumes all consecutive texture coordinate lines"""
+        # The first iteration processes the current/first vn statement.
+        # The loop continues until there are no more vn-statements or StopIteration is raised by generator
         while True:
             yield (
                 float(self.values[1]),
@@ -83,6 +85,8 @@ def parse_vt(self):
 
     def consume_texture_coordinates(self):
         """Consume all consecutive texture coordinates"""
+        # The first iteration processes the current/first vt statement.
+        # The loop continues until there are no more vt-statements or StopIteration is raised by generator
         while True:
             yield (
                 float(self.values[1]),
@@ -153,6 +157,8 @@ def consume_faces(self):
 
         We always triangulate to make it simple
         """
+        # The first iteration processes the current/first f statement.
+        # The loop continues until there are no more f-statements or StopIteration is raised by generator
         while True:
             v1 = None
             vlast = None

pywavefront/parser.py

@@ -33,7 +33,6 @@
 # ----------------------------------------------------------------------------
 import codecs
 import gzip
-import io
 import logging
 import os
 import sys
@@ -75,49 +74,44 @@ def create_line_generator(self):
         Creates a generator function yielding lines in the file
         Should only yield non-empty lines
         """
+
         if self.file_name.endswith(".gz"):
             if sys.version_info.major == 3:
                 gz = gzip.open(self.file_name, mode='rt', encoding=self.encoding)
-                f = io.BufferedReader(gz)
-                for line in gz:
-                    if line == '':
-                        continue
-                    yield line
-
-                gz.close()
             else:
                 gz = gzip.open(self.file_name, mode='rt')
-                f = io.BufferedReader(gz)
-                for line in f.readlines():
-                    if line == '':
-                        continue
-                    yield line
 
-                gz.close()
+            for line in gz.readlines():
+                if line == '':
+                    continue
+                yield line
+
+            gz.close()
         else:
             if sys.version_info.major == 3:
                 # Python 3 native `open` is much faster
-                with open(self.file_name, mode='r', encoding=self.encoding) as file:
-                    for line in file:
-                        # Skip empty lines
-                        if line == '':
-                            continue
-                        yield line
+                file = open(self.file_name, mode='r', encoding=self.encoding)
             else:
-                with codecs.open(self.file_name, mode='r', encoding=self.encoding) as file:
-                    for line in file:
-                        # Skip empty lines
-                        if line == '':
-                            continue
-                        yield line
+                # Python 2 needs the codecs package to deal with encoding
+                file = codecs.open(self.file_name, mode='r', encoding=self.encoding)
+
+            for line in file:
+                if line == '':  # Skip empty lines
+                    continue
+                yield line
+
+            file.close()
 
     def next_line(self):
         """Read the next line from the line generator and split it"""
-        self.line = next(self.lines)
+        self.line = next(self.lines)  # Will raise StopIteration when there are no more lines
         self.values = self.line.split()
 
     def consume_line(self):
-        """Tell the parser we are done with this line"""
+        """
+        Tell the parser we are done with this line.
+        This is simply by setting None values.
+        """
         self.line = None
         self.values = None
 
@@ -127,8 +121,12 @@ def parse(self):
         Determines what type of line we are and dispatch appropriately.
         """
         try:
+            # Continues until `next_line()` raises StopIteration
+            # This can trigger here or in parse functions in the subclass
             while True:
-                # Only advance the parser if the previous line was consumed
+                # Only advance the parser if the previous line was consumed.
+                # Parse functions reading multiple lines can end up reading one line too far,
+                # so they return without consuming the line and we pick it up here
                 if not self.line:
                     self.next_line()
 
@@ -150,8 +148,6 @@ def parse_fallback(self):
             logging.warning("Unimplemented OBJ format statement '%s' on line '%s'"
                             % (self.values[0], self.line.rstrip()))
 
-        self.consume_line()
-
     def _build_dispatch_map(self):
         """
         Build a dispatch map: {func name": func} dict

pywavefront/visualization.py

@@ -46,10 +46,6 @@ def draw(instance):
         draw_mesh(instance)
     elif isinstance(instance, dict):
         draw_meshes(instance)
-    elif isinstance(instance, list):
-        draw_meshes(instance)
-    elif isinstance(instance, tuple):
-        draw_meshes(instance)
     else:
         raise ValueError("Cannot figure out how to draw: {}".format(instance))
 

test/test_wavefront.py

@@ -39,7 +39,7 @@ def testAddDuplicateMesh(self):
         self.assertEqual(len(self.meshes.mesh_list), len(self.mesh_names) + 1)
 
     def testMeshMaterialVertices(self):
-        "" "Mesh vertices should have known values."""
+        """Mesh vertices should have known values."""
         self.assertEqual(len(self.meshes.meshes[self.mesh_names[0]].materials[0].vertices), 24)
 
 
@@ -51,11 +51,6 @@ def testUnknownUsemtl(self):
                           pywavefront.Wavefront,
                           prepend_dir('simple_unknown_usemtl.obj'))
 
-    # def testMissingNormals(self):
-    #     """If there are texture coordinates but no normals, should raise an exception."""
-    #     self.assertRaises(pywavefront.PywavefrontException,
-    #                       pywavefront.Wavefront, prepend_dir('simple_missing_normals.obj'))
-
 
 class TestNoMaterial(TestWavefront):
     def setUp(self):