Issue 4867 - Add global get_pages_metadata to template converters

cms/converters.py

 # This file is part of the Adblock Plus web scripts,
 # Copyright (C) 2006-2017 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
@@ skipping 98 matching lines @@
         # the document.
         self._append_text(data)
 
     def handle_entityref(self, name):
         self._append_text(self.unescape('&{};'.format(name)))
 
     def handle_charref(self, name):
         self._append_text(self.unescape('&#{};'.format(name)))
 
 
+def get_page_metadata(page, data):
+    """Generator which gets per page metadata and corresponding line indices"""
+    for i, line in enumerate(data.splitlines()):
+        if not re.search(r'^\s*[\w\-]+\s*=', line):
+            break
+        name, value = line.split('=', 1)
+        value = value.strip()
+        if value.startswith('[') and value.endswith(']'):
+            value = [element.strip() for element in value[1:-1].split(',')]
+        yield name.strip(), value, i
+
+
 class Converter:
     whitelist = {'a', 'em', 'sup', 'strong', 'code', 'span'}
     missing_translations = 0
     total_translations = 0
 
     def __init__(self, params, key='pagedata'):
         self._params = params
         self._key = key
         self._attribute_parser = AttributeParser(self.whitelist)
         self._seen_defaults = {}
 
         # Read in any parameters specified at the beginning of the file
         data, filename = params[key]
         lines = data.splitlines(True)
-        for i, line in enumerate(lines):
-            if not re.search(r'^\s*[\w\-]+\s*=', line):
-                break
-            name, value = line.split('=', 1)
-            params[name.strip()] = value.strip()
+        for name, value, i in get_page_metadata(params['page'], data):

[Vasily Kuznetsov, 2017/06/27 13:32:15]

This refactoring moved the code into a separate function, but the code is still
very interdependent: this `i` in return tuples has nothing to do with metadata
and why are we even returning an iterator of tuples, wouldn't it be better to
return a dictionary instead.
I propose that we go one step further. How about we change `get_page_metadata`
to `parse_page(data) -> (metadata, pagecontent)` (where `metadata` is a
dictionary and `pagecontent` is a string). This function will have all parsing
logic in it, from splitting lines to making the dictionary out of metadata. Then
we can just `params.update(metadata)` and `params[key] = (pagecontent,
filename)`.

[Jon Sonesen, 2017/06/28 14:31:19]

The line index is required to strip the metadata from the page data which gets
inserted to params['page_content'].

[Jon Sonesen, 2017/07/03 09:06:06]

Done.

+            params[name] = value
             lines[i] = '\n'
         params[key] = (''.join(lines), filename)
 
     def localize_string(
             self, page, name, default, comment, localedata, escapes):
 
         def escape(s):
             return re.sub(r'.',
                           lambda match: escapes.get(match.group(0),
                                                     match.group(0)),
@@ skipping 228 matching lines @@
         filters = {
             'translate': self.translate,
             'linkify': self.linkify,
             'toclist': self.toclist,
         }
 
         globals = {
             'get_string': self.get_string,
             'has_string': self.has_string,
             'get_page_content': self.get_page_content,
+            'get_pages_metadata': self.get_pages_metadata,
         }
 
         for dirname, dictionary in [('filters', filters),
                                     ('globals', globals)]:
             for filename in self._params['source'].list_files(dirname):
                 root, ext = os.path.splitext(filename)
                 if ext.lower() != '.py':
                     continue
 
                 path = os.path.join(dirname, filename)
@@ skipping 68 matching lines @@
             locale = self._params['locale']
 
         locale, url = self._params['source'].resolve_link(page, locale)
         return jinja2.Markup('<a{}>'.format(''.join(
             ' {}="{}"'.format(name, jinja2.escape(value)) for name, value in [
                 ('href', url),
                 ('hreflang', locale)
             ] + attrs.items()
         )))
 
+    def get_pages_metadata(self, filters=None):
+        if not isinstance(filters, dict) and filters:

[Vasily Kuznetsov, 2017/06/27 13:32:15]

This code seems to allow things like `filters = []` or `filters = 0` (which will
crash inside of `filter_metadata`). The check should probably be something like
`if filters is not None and not isinstance(...):` instead.

[Jon Sonesen, 2017/06/28 14:31:19]

Acknowledged.

[Jon Sonesen, 2017/07/03 09:06:06]

Done.

+            raise TypeError('Filters are not a dictionary')
+
+        return_data = []
+        for page_name, _format in self._params['source'].list_pages():
+            data, filename = self._params['source'].read_page(page_name,
+                                                              _format)
+            page_data = {'page': page_name}
+            for name, value, i in get_page_metadata(page_name, data):
+                page_data[name] = value
+            if self.filter_metadata(filters, page_data) is True:
+                return_data.append(page_data)
+        return return_data
+
+    def filter_metadata(self, filters, metadata):
+        # if only the page key is in the metadata then there
+        # was no user defined metadata
+        if metadata.keys() == ['page']:

[Vasily Kuznetsov, 2017/06/27 13:32:15]

Is this a requirement that such metadata should be ignored? This is an honest
question, I see why you might want to do this, but I can also imagine use cases
for including those pages.

[Jon Sonesen, 2017/06/28 14:31:19]

This is done to keep a uniform api, the previous implementation does not include
files which have no user defined metadata so adding it now may cause a
regression, however this is probably best directed to @julian since he is the
user.

[Jon Sonesen, 2017/07/03 09:06:06]

Still waiting on julians opinion, i guess i get whaat you mean here since have
the pages a member regardless of their metadta could be useful

[juliandoucette, 2017/07/03 21:48:51]

I don't understand the question. Can you please explain?

[Vasily Kuznetsov, 2017/07/04 07:43:48]

The question is: if a page has no explicit metadata (so it will only have 'page'
key in the dictionary) but matches the query (probably means that the query was
empty), should it be in the results? Current implementation always filters out
pages with no explicit metadata.

[juliandoucette, 2017/07/04 09:57:24]

Or that I queried by page name?
So I would get an object like this?

{
  "page": "name"
  "content": "..."
}

If so, why would I want to filter this out?

[Vasily Kuznetsov, 2017/07/04 10:23:34]

The content is not in the dictionary currently. It can be added, but I would say
that should be another ticket. In any case, I was doubting that you would want
to filter such pages out, thanks for confirming that.

Jon: I would say this `if` should be removed.

+            return False
+        if filters is None:
+            return True
+        for filter_name, filter_value in filters.items():
+            if filter_name not in metadata:
+                return False
+            if isinstance(metadata[filter_name], list):
+                if isinstance(filter_value, basestring):
+                    filter_value = [filter_value]
+                for option in filter_value:
+                    if str(option) not in metadata[filter_name]:
+                        return False
+            elif filter_value != metadata[filter_name]:
+                    return False
+        return True
+
     def toclist(self, content):
         toc_re = r'<h(\d)\s[^<>]*\bid="([^<>"]+)"[^<>]*>(.*?)</h\1>'
         flat = []
         for match in re.finditer(toc_re, content, re.S):
             flat.append({
                 'level': int(match.group(1)),
                 'anchor': jinja2.Markup(match.group(2)).unescape(),
                 'title': jinja2.Markup(match.group(3)).unescape(),
                 'subitems': [],
             })
 
         structured = []
         stack = [{'level': 0, 'subitems': structured}]
         for item in flat:
             while stack[-1]['level'] >= item['level']:
                 stack.pop()
             stack[-1]['subitems'].append(item)
             stack.append(item)
         return structured
 
 converters = {
     'html': RawConverter,
     'md': MarkdownConverter,
     'tmpl': TemplateConverter,
 }