Fixed regex condition. Fixed unclosed files.

Added TODO comments

src/libkomwm.py

@@ -89,6 +89,7 @@ Priorities ranges' rendering order overview:
 - BG-by-size: landcover areas sorted by their size
 '''
 
+# TODO: Implement better error handling
 validation_errors_count = 0
 
 def to_boolean(s):
@@ -104,6 +105,9 @@ def mwm_encode_color(colors, st, prefix='', default='black'):
     if prefix:
         prefix += "-"
     opacity = hex(255 - int(255 * float(st.get(prefix + "opacity", 1))))
+    # TODO: Refactoring idea: here color is converted from float to hex. While MapCSS class
+    #       reads colors from *.mapcss files and converts to float. How about changing MapCSS
+    #       to keep hex values and avoid Hex->Float->Hex operations?
     color = whatever_to_hex(st.get(prefix + 'color', default))[1:]
     result = int(opacity + color, 16)
     colors.add(result)
@@ -118,6 +122,7 @@ def mwm_encode_image(st, prefix='icon', bgprefix='symbol'):
         return False
     # strip last ".svg"
     handle = st.get(prefix + "image")[:-4]
+    # TODO: return `handle` only once
     return handle, handle
 
 
@@ -454,6 +459,7 @@ def get_drape_priority(cl, dr_type, object_id, auto_dr_type = None, auto_comment
     return 0
 
 
+# TODO: Split large function to smaller ones
 def komap_mapswithme(options):
     if options.data and os.path.isdir(options.data):
         ddir = options.data
@@ -464,6 +470,7 @@ def komap_mapswithme(options):
     class_order = []
     class_tree = {}
 
+    # TODO: Introduce new function to parse `colors.txt` for better testability
     colors_file_name = os.path.join(ddir, 'colors.txt')
     colors = set()
     if os.path.exists(colors_file_name):
@@ -472,6 +479,7 @@ def komap_mapswithme(options):
             colors.add(int(colorLine))
         colors_in_file.close()
 
+    # TODO: Introduce new function to parse `patterns.txt` for better testability
     patterns = []
     def addPattern(dashes):
         if dashes and dashes not in patterns:
@@ -488,9 +496,11 @@ def komap_mapswithme(options):
     types_file = open(os.path.join(ddir, 'types.txt'), "w")
 
     # The mapcss-mapping.csv format is described inside the file itself.
+    # TODO: introduce new function to parse 'mapcss-mapping.csv' for better testability
     cnt = 1
     unique_types_check = set()
-    for row in csv.reader(open(os.path.join(ddir, 'mapcss-mapping.csv')), delimiter=';'):
+    mapping_file = open(os.path.join(ddir, 'mapcss-mapping.csv'))
+    for row in csv.reader(mapping_file, delimiter=';'):
         if len(row) <= 1 or row[0].startswith('#'):
             # Allow for empty lines and comment lines starting with '#'.
             continue
@@ -537,6 +547,7 @@ def komap_mapswithme(options):
                 print("mapswithme", file=types_file)
         class_tree[cl] = row[0]
     class_order.sort()
+    mapping_file.close()
     types_file.close()
 
     output = ''
@@ -554,8 +565,10 @@ def komap_mapswithme(options):
         for i, t in enumerate(v.keys()):
             mapcss_static_tags[t] = mapcss_static_tags.get(t, True) and i == 0
 
+    # TODO: Introduce new function to parse `mapcss-dynamic.txt` for better testability
     # Get all mapcss dynamic tags from mapcss-dynamic.txt
-    mapcss_dynamic_tags = set([line.rstrip() for line in open(os.path.join(ddir, 'mapcss-dynamic.txt'))])
+    with open(os.path.join(ddir, 'mapcss-dynamic.txt')) as dynamic_file:
+        mapcss_dynamic_tags = set([line.rstrip() for line in dynamic_file])
 
     # Parse style mapcss
     global style
@@ -579,6 +592,7 @@ def komap_mapswithme(options):
 
     style.finalize_choosers_tree()
 
+    # TODO: Introduce new function to work with colors for better testability
     # Get colors section from style
     style_colors = {}
     raw_style_colors = style.get_colors()
@@ -616,6 +630,7 @@ def komap_mapswithme(options):
 
     all_draw_elements = set()
 
+    # TODO: refactor next for-loop for readability and testability
     global validation_errors_count
     for results in imapfunc(query_style, ((cl, classificator[cl], options.minzoom, options.maxzoom) for cl in class_order)):
         for result in results:
@@ -920,6 +935,7 @@ def komap_mapswithme(options):
         return -1
     viskeys.sort(key=functools.cmp_to_key(cmprepl))
 
+    # TODO: Introduce new function to dump `visibility.txt` and `classificator.txt` for better testability
     visibility_file = open(os.path.join(ddir, 'visibility.txt'), "w")
     classificator_file = open(os.path.join(ddir, 'classificator.txt'), "w")
 
@@ -942,11 +958,13 @@ def komap_mapswithme(options):
     visibility_file.close()
     classificator_file.close()
 
+    # TODO: Introduce new function to dump `colors.txt` for better testability
     colors_file = open(colors_file_name, "w")
     for c in sorted(colors):
         colors_file.write("%d\n" % (c))
     colors_file.close()
 
+    # TODO: Introduce new function to dump `patterns.txt` for better testability
     patterns_file = open(patterns_file_name, "w")
     for p in patterns:
         patterns_file.write("%s\n" % (' '.join(str(elem) for elem in p)))

src/mapcss/Condition.py

@@ -24,7 +24,7 @@ class Condition:
             params = (params,)
         self.params = params      # e.g. ('highway','primary')
         if typez == "regex":
-            self.regex = re.compile(self.params[0], re.I)
+            self.regex = re.compile(self.params[1], re.I)
 
     def extract_tag(self):
         if self.params[0][:2] == "::" or self.type == "regex":

src/mapcss/Eval.py

@@ -15,6 +15,10 @@
 #   You should have received a copy of the GNU General Public License
 #   along with kothic.  If not, see <http://www.gnu.org/licenses/>.
 
+import logging
+
+logger = logging.getLogger('mapcss.Eval')
+logger.setLevel(logging.ERROR)
 
 class Eval():
     def __init__(self, s='eval()'):
@@ -57,6 +61,8 @@ class Eval():
                  "any": fake_compute,
                  "min": fake_compute,
                  "max": fake_compute,
+                 "cond": fake_compute,
+                 "boolean": fake_compute,
                  })
         return tags
 
@@ -99,12 +105,15 @@ class Eval():
                 return "{:.4g}".format(result)
 
             return str(result)
-        except:
+        except Exception as e:
+            logger.warning(f"Error evaluating expression `{self.expr_text}`", e)
             return ""
 
     def __repr__(self):
         return "eval(%s)" % self.expr_text
 
+    def __eq__(self, other):
+        return type(self) == type(other) and self.expr_text == other.expr_text
 
 def m_boolean(expr):
     expr = str(expr)

src/mapcss/StyleChooser.py

@@ -76,6 +76,8 @@ class StyleChooser:
     The styles property is an array of all the style objects to be drawn
         if any of the ruleChains evaluate to true.
     """
+    # TODO: use logging for debug logs
+
     def __repr__(self):
         return "{(%s) : [%s] }\n" % (self.ruleChains, self.styles)
 
@@ -122,6 +124,7 @@ class StyleChooser:
 
         return rule.runtime_conditions
 
+    # TODO: Rename to "applyStyles"
     def updateStyles(self, sl, tags, xscale, zscale, filter_by_runtime_conditions):
         # Are any of the ruleChains fulfilled?
         rule_and_object_id = self.testChains(tags)
@@ -143,6 +146,7 @@ class StyleChooser:
                 for a, b in r.items():
                     "calculating eval()'s"
                     if type(b) == self.eval_type:
+                        # TODO: Move next block to a separate function
                         combined_style = {}
                         for t in sl:
                             combined_style.update(t)
@@ -235,6 +239,7 @@ class StyleChooser:
         """
         adds to this.styles
         """
+        # TODO: move next for-loop to a new method. Don't call it on every style append
         for r in self.ruleChains:
             if not self.selzooms:
                 self.selzooms = [r.minZoom, r.maxZoom]

src/mapcss/__init__.py

@@ -25,6 +25,7 @@ from .Condition import Condition
 NEEDED_KEYS = set(["width", "casing-width", "casing-width-add", "fill-color", "fill-image", "icon-image", "text", "extrude",
                    "background-image", "background-color", "pattern-image", "shield-color", "symbol-shape"])
 
+# TODO: Unused constant
 WHITESPACE = re.compile(r'\s+ ', re.S | re.X)
 
 COMMENT = re.compile(r'\/\* .*? \*\/ \s* ', re.S | re.X)
@@ -40,29 +41,30 @@ VARIABLE_SET = re.compile(r'@([a-z][\w\d]*) \s* : \s* (.+?) \s* ; \s* ', re.S |
 UNKNOWN = re.compile(r'(\S+) \s* ', re.S | re.X)
 
 ZOOM_MINMAX = re.compile(r'(\d+)\-(\d+) $', re.S | re.X)
-ZOOM_MIN = re.compile(r'(\d+)\-      $', re.S | re.X)
+ZOOM_MIN    = re.compile(r'(\d+)\-      $', re.S | re.X)
-ZOOM_MAX = re.compile(r'     \-(\d+) $', re.S | re.X)
+ZOOM_MAX    = re.compile(r'     \-(\d+) $', re.S | re.X)
 ZOOM_SINGLE = re.compile(r'       (\d+) $', re.S | re.X)
 
-CONDITION_TRUE = re.compile(r'\s* ([:\w]+) \s* [?] \s*  $', re.I | re.S | re.X)
+# TODO: move to Condition.py
+CONDITION_TRUE    = re.compile(r'\s* ([:\w]+) \s* [?] \s*  $', re.I | re.S | re.X)
 CONDITION_invTRUE = re.compile(r'\s* [!] \s* ([:\w]+) \s* [?] \s*  $', re.I | re.S | re.X)
-CONDITION_FALSE = re.compile(r'\s* ([:\w]+) \s* = \s* no  \s*  $', re.I | re.S | re.X)
+CONDITION_FALSE   = re.compile(r'\s* ([:\w]+) \s* = \s* no  \s*  $', re.I | re.S | re.X)
-CONDITION_SET = re.compile(r'\s* ([-:\w]+) \s* $', re.S | re.X)
+CONDITION_SET     = re.compile(r'\s* ([-:\w]+) \s* $', re.S | re.X)
-CONDITION_UNSET = re.compile(r'\s* !([:\w]+) \s* $', re.S | re.X)
+CONDITION_UNSET   = re.compile(r'\s* !([:\w]+) \s* $', re.S | re.X)
-CONDITION_EQ = re.compile(r'\s* ([:\w]+) \s* =  \s* (.+) \s* $', re.S | re.X)
+CONDITION_EQ      = re.compile(r'\s* ([:\w]+) \s* =  \s* (.+) \s* $', re.S | re.X)
-CONDITION_NE = re.compile(r'\s* ([:\w]+) \s* != \s* (.+) \s* $', re.S | re.X)
+CONDITION_NE      = re.compile(r'\s* ([:\w]+) \s* != \s* (.+) \s* $', re.S | re.X)
-CONDITION_GT = re.compile(r'\s* ([:\w]+) \s* >  \s* (.+) \s* $', re.S | re.X)
+CONDITION_GT      = re.compile(r'\s* ([:\w]+) \s* >  \s* (.+) \s* $', re.S | re.X)
-CONDITION_GE = re.compile(r'\s* ([:\w]+) \s* >= \s* (.+) \s* $', re.S | re.X)
+CONDITION_GE      = re.compile(r'\s* ([:\w]+) \s* >= \s* (.+) \s* $', re.S | re.X)
-CONDITION_LT = re.compile(r'\s* ([:\w]+) \s* <  \s* (.+) \s* $', re.S | re.X)
+CONDITION_LT      = re.compile(r'\s* ([:\w]+) \s* <  \s* (.+) \s* $', re.S | re.X)
-CONDITION_LE = re.compile(r'\s* ([:\w]+) \s* <= \s* (.+) \s* $', re.S | re.X)
+CONDITION_LE      = re.compile(r'\s* ([:\w]+) \s* <= \s* (.+) \s* $', re.S | re.X)
-CONDITION_REGEX = re.compile(r'\s* ([:\w]+) \s* =~\/ \s* (.+) \/ \s* $', re.S | re.X)
+CONDITION_REGEX   = re.compile(r'\s* ([:\w]+) \s* =~\/ \s* (.+) \/ \s* $', re.S | re.X)
 
 ASSIGNMENT_EVAL = re.compile(r"\s* (\S+) \s* \:      \s* eval \s* \( \s* ' (.+?) ' \s* \) \s* $", re.I | re.S | re.X)
-ASSIGNMENT = re.compile(r'\s* (\S+) \s* \:      \s*          (.+?) \s*                   $', re.S | re.X)
+ASSIGNMENT      = re.compile(r'\s* (\S+) \s* \:      \s*          (.+?) \s*                   $', re.S | re.X)
-SET_TAG_EVAL = re.compile(r"\s* set \s+(\S+)\s* = \s* eval \s* \( \s* ' (.+?) ' \s* \) \s* $", re.I | re.S | re.X)
+SET_TAG_EVAL    = re.compile(r"\s* set \s+(\S+)\s* = \s* eval \s* \( \s* ' (.+?) ' \s* \) \s* $", re.I | re.S | re.X)
-SET_TAG = re.compile(r'\s* set \s+(\S+)\s* = \s*          (.+?) \s*                   $', re.I | re.S | re.X)
+SET_TAG         = re.compile(r'\s* set \s+(\S+)\s* = \s*          (.+?) \s*                   $', re.I | re.S | re.X)
-SET_TAG_TRUE = re.compile(r'\s* set \s+(\S+)\s* $', re.I | re.S | re.X)
+SET_TAG_TRUE    = re.compile(r'\s* set \s+(\S+)\s* $', re.I | re.S | re.X)
-EXIT = re.compile(r'\s* exit \s* $', re.I | re.S | re.X)
+EXIT            = re.compile(r'\s* exit \s* $', re.I | re.S | re.X)
 
 oNONE = 0
 oZOOM = 2
@@ -73,6 +75,7 @@ oDECLARATION = 6
 oSUBPART = 7
 oVARIABLE_SET = 8
 
+# TODO: Following block of variables is never used
 DASH = re.compile(r'\-/g')
 COLOR = re.compile(r'color$/')
 BOLD = re.compile(r'^bold$/i')
@@ -81,6 +84,7 @@ UNDERLINE = re.compile(r'^underline$/i')
 CAPS = re.compile(r'^uppercase$/i')
 CENTER = re.compile(r'^center$/i')
 
+# TODO: Remove unused HEX variable
 HEX = re.compile(r'^#([0-9a-f]+)$/i')
 VARIABLE = re.compile(r'@([a-z][\w\d]*)')
 
@@ -98,6 +102,7 @@ class MapCSS():
         self.choosers_by_type = {}
         self.choosers_by_type_zoom_tag = {}
         self.variables = {}
+        self.unused_variables = set()
         self.style_loaded = False
 
     def parseZoom(self, s):
@@ -110,6 +115,7 @@ class MapCSS():
         elif ZOOM_SINGLE.match(s):
             return float(ZOOM_SINGLE.match(s).groups()[0]), float(ZOOM_SINGLE.match(s).groups()[0])
         else:
+            # TODO: Should we raise an exception here?
             logging.error("unparsed zoom: %s" % s)
 
     def build_choosers_tree(self, clname, type, cltag):
@@ -163,6 +169,8 @@ class MapCSS():
                     runtime_rules.append(runtime_conditions)
         return runtime_rules
 
+    # TODO: Renamed to `get_styles` because it returns a list of styles for each class `::XXX`
+    # Refactoring idea: Maybe return dict with `object-id` as a key
     def get_style(self, clname, type, tags, zoom, xscale, zscale, filter_by_runtime_conditions):
         style = []
         if type in self.choosers_by_type_zoom_tag:
@@ -207,6 +215,8 @@ class MapCSS():
 
     def get_variable(self, m):
         name = m.group()[1:]
+        if name in self.unused_variables:
+            self.unused_variables.remove(name)
         if not name in self.variables:
             raise Exception("Variable not found: " + str(format(name)))
         return self.variables[name] if name in self.variables else m.group()
@@ -219,7 +229,8 @@ class MapCSS():
         if filename:
             basepath = os.path.dirname(filename)
         if not css:
-            css = open(filename).read()
+            with open(filename) as css_file:
+                css = css_file.read()
         if not self.style_loaded:
             self.choosers = []
 
@@ -339,7 +350,8 @@ class MapCSS():
                         import_filename = os.path.join(basepath, IMPORT.match(css).groups()[0])
                         try:
                             css = IMPORT.sub("", css, 1)
-                            import_text = open(import_filename, "r").read()
+                            with open(import_filename, "r") as import_file:
+                                import_text = import_file.read()
                             stck[-1][1] = css # store remained part
                             stck.append([import_filename, import_text, import_text])
                             wasBroken = True
@@ -352,6 +364,7 @@ class MapCSS():
                         name = VARIABLE_SET.match(css).groups()[0]
                         log.debug("variable set found: %s" % name)
                         self.variables[name] = VARIABLE_SET.match(css).groups()[1]
+                        self.unused_variables.add( name )
                         css = VARIABLE_SET.sub("", css, 1)
                         previous = oVARIABLE_SET
 
@@ -359,7 +372,7 @@ class MapCSS():
                     elif UNKNOWN.match(css):
                         raise Exception("Unknown construction: " + UNKNOWN.match(css).group())
 
-                    # Must be unreacheable
+                    # Must be unreachable
                     else:
                         raise Exception("Unexpected construction: " + css)
 
@@ -377,10 +390,13 @@ class MapCSS():
             css_orig = stck[-1][2] # original
             css = stck[-1][1] # remained
             line = css_orig[:-len(css)].count("\n") + 1
+            # TODO: Handle filename is None
             msg = str(e) + "\nFile: " + filename + "\nLine: " + str(line)
+            # TODO: Print stack trace of original exception `e`
             raise Exception(msg)
 
         try:
+            # TODO: Drop support of z-index because `clamp` is always False and z-index properties unused in Organic Maps)
             if clamp:
                 "clamp z-indexes, so they're tightly following integers"
                 zindex = set()
@@ -398,8 +414,10 @@ class MapCSS():
                             else:
                                 stylez['z-index'] = res
         except TypeError:
+            # TODO: Better error handling here
             pass
 
+        # Group MapCSS styles by object type: 'area', 'line', 'way', 'node'
         for chooser in self.choosers:
             for t in chooser.compatible_types:
                 if t not in self.choosers_by_type:
@@ -407,7 +425,11 @@ class MapCSS():
                 else:
                     self.choosers_by_type[t].append(chooser)
 
+        if self.unused_variables:
+            # TODO: Do not print warning here. Instead let libkomwn.komap_mapswithme(...) analyze unused_variables
+            print(f"Warning: Unused variables: {', '.join(self.unused_variables)}")
 
+# TODO: move to Condition.py
 def parseCondition(s):
     log = logging.getLogger('mapcss.parser.condition')
 
@@ -488,7 +510,7 @@ def parseDeclaration(s):
             logging.debug("%s == %s" % (tzz[0], tzz[1]))
         else:
             logging.debug("unknown %s" % (a))
-    return [t]
+    return [t] # TODO: don't wrap `t` dict into a list. Return `t` instead.
 
 
 if __name__ == "__main__":