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()):
+def parse_page_content(page, data):
+    """Separate page content into metadata (dict) and body text (str)"""
+    page_data = {'page': page}
+    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)
         value = value.strip()
         if value.startswith('[') and value.endswith(']'):
             value = [element.strip() for element in value[1:-1].split(',')]
-        yield name.strip(), value, i
+        lines[i] = '\n'
+        page_data[name.strip()] = value
+    return page_data, ''.join(lines)
 
 
 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
+        # and override converter defaults with page specific params
         data, filename = params[key]
-        lines = data.splitlines(True)
-        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)
+        page_data, body_text = parse_page_content(params['page'], data)
+        params.update(page_data)
+        params[key] = (body_text, 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)),
                           s, flags=re.S)
 
@@ skipping 316 matching lines @@
 
         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.

+        if filters is not None and not isinstance(filters, dict):
             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
+            page_data = parse_page_content(page_name, data)[0]
             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:
@@ skipping 21 matching lines @@
                 stack.pop()
             stack[-1]['subitems'].append(item)
             stack.append(item)
         return structured
 
 converters = {
     'html': RawConverter,
     'md': MarkdownConverter,
     'tmpl': TemplateConverter,
 }