Issue 1170 - [adblockplus.org Anwiki to CMS migration] Migrate content

filters/parse_interface.py

-import re
+# This file is part of the Adblock Plus website,
+# Copyright (C) 2006-2015 Eyeo GmbH
+#
+# Adblock Plus is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License version 3 as
+# published by the Free Software Foundation.
+#
+# Adblock Plus is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
 
-def parse_interface(interfaces):

[Wladimir Palant, 2014/09/18 20:33:04]

Nit: I think what it gets as parameter isn't interfaces but rather items inside
an interface.

[kzar, 2014/09/19 11:28:06]

Done.

+import re, warnings
+
+TYPE_REGEXP = r"(?:arrayof\s+)?\w+"
+
+def parse_interface(interface_items):
   parsed = []
-  for property_key, property in interfaces.iteritems():

[Wladimir Palant, 2014/09/18 20:33:04]

Nit: I think that simple key, value as names would have been better here...

[kzar, 2014/09/19 11:28:06]

Done.

-    if property_key.find("(") > -1:

[Wladimir Palant, 2014/09/18 20:33:04]

Nit: this can be written as simply |"(" in property_key|. Also, you might want
to add a comment here, along the lines of:

# This is a method

[kzar, 2014/09/19 11:28:06]

Done.

-      match = re.match("^(arrayof\W+\w+|\w+)\W+(\w+)[\W\(]+([^\)]*)[\W\)]+$", property_key)

[Wladimir Palant, 2014/09/18 20:33:04]

You generally want to use r"foo" rather than "foo" for regular expressions.
Please compare |print "\b"| vs. |print r"\b"|.

Also, you probably want to add a constant TYPE_REGEXP = r"(?:arrayof\s+)?\w+"
and insert it here rather than duplicating the regular expression for types
every time. Note also that you should use \s+ rather than \W+ when you mean some
number of spaces.

Finally, [\W\(] should really be \s*\(\s* and [\W\)]+ should be \s*\)\s*. While
at it, you probably want to add \s* at the beginning of the regexp as well to
allow leading spaces.

[kzar, 2014/09/19 11:28:06]

Done.

+  for key, value in interface_items.iteritems():
+    if "(" in key:
+      # Item is a method
+      match = re.match(r"^\s*(%s)\s+(\S+)\s*\(\s*([^\)]*)\s*\)\s*$" % TYPE_REGEXP, key)
       if not match:
+        warnings.warn("Skipped malformed method: '%s'" % key)
         continue

[Wladimir Palant, 2014/09/18 20:33:04]

Just dropping the line without any message is a great footgun, you won't notice
that your syntax is off. How about using warnings.warn() here?

[kzar, 2014/09/19 11:28:06]

Done.

       return_type, property_name, argument_string = match.groups()
       arguments = []
-      if "version" in property:
-        property_version = property["version"]
-      else:
-        property_version = ""

[Wladimir Palant, 2014/09/18 20:33:04]

Nit: Use None instead of empty string here?

[kzar, 2014/09/19 11:28:06]

I think empty strings are more useful for templates. If a variable is set to
None I require extra logic in the template to check, with an empty string I can
choose to just display it, and if required it's still falsey for conditionals.

On 2014/09/18 20:33:04, Wladimir Palant wrote:

       if argument_string:
-        for i, argument in enumerate(argument_string.split(",")):

[Wladimir Palant, 2014/09/18 20:33:04]

Nit: Why use enumerate() here? Doesn't seem to be needed.

[kzar, 2014/09/19 11:28:06]

Done.

-          parts = argument.split(" ")

[Wladimir Palant, 2014/09/18 20:33:04]

How about:

  match = re.match(r"^\s*(%s)\s+(\w+)\s*$" % TYPE_REGEXP, argument)

Then you can actually validate the input. Side-note: your code is equivalent to:

  argument_type, argument_name = argument.rsplit(maxsplit=1)

[kzar, 2014/09/19 11:28:06]

Done.

-          argument_name = parts[-1]
-          argument_type = " ".join(parts[:-1])
-          arguments.append({
-            "name": argument_name,
-            "type": argument_type
-          })
-      parsed.append({
+        for argument in argument_string.split(","):
+          if argument.strip():
+            match = re.match(r"^\s*(%s)\s+(\S+)\s*$" % TYPE_REGEXP, argument)
+            if not match:
+              warnings.warn("Skipped malformed argument: '%s'" % argument)
+              continue
+            argument_type, argument_name = match.groups()
+            arguments.append({
+              "name": argument_name,
+              "type": argument_type
+            })
+      value.update({
         "type": "method",
         "name": property_name,
-        "version": property_version,
         "return_type": return_type,
-        "arguments": arguments,
+        "arguments": arguments
       })
+      parsed.append(value)
     else:
-      parts = property_key.split()

[Wladimir Palant, 2014/09/18 20:33:04]

I'd prefer:

  match = re.match(r"^\s*(readonly\s+)?(%s)\s+(\w+)\s*$" % TYPE_REGEXP,
argument)

Again, this way the property specification is validated and you can warn if you
encounter something unexpected.

[kzar, 2014/09/19 11:28:06]

Done.

-      property = { }

[Wladimir Palant, 2014/09/18 20:33:04]

Nit: no space between the brackets.

[kzar, 2014/09/19 11:28:06]

Done.

-      property["type"], property["name"] = parts[-2:]
-      if len(parts) == 3:
-        property["modifier"] = parts[0]

[Wladimir Palant, 2014/09/18 20:33:04]

I think using temporary variables like you've done it for the methods is a more
readable approach.

[kzar, 2014/09/19 11:28:06]

Done.

-      parsed.append(property)
-  parsed = sorted(parsed, key=lambda x: x["name"])

[Wladimir Palant, 2014/09/18 20:33:04]

Please use parse.sort() that will sort the list in-place, without creating a new
one.

[kzar, 2014/09/19 11:28:06]

Done.

+      # Item is a property
+      match = re.match(r"^\s*(readonly\s+)?(%s)\s+(\S+)\s*$" % TYPE_REGEXP, key)
+      if not match:
+        warnings.warn("Skipped malformed property: '%s'" % key)
+        continue
+      property_modifier, property_type, property_name = match.groups()
+      value.update({
+        "type": property_type,
+        "name": property_name,
+        "modifier": property_modifier or ""
+      })
+      parsed.append(value)
+  parsed.sort(key=lambda x: x["name"])
   return parsed