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 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(',')]
+        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 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()
-            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:
+        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 = self.parse_page_metadata(data, page_name)
-
+            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 parse_page_metadata(self, data, page):

[Jon Sonesen, 2017/06/23 10:09:26]

This code is essentially duplicating the logic in the init function of the
Converter class, Vasily and I discussed this and the options were to break the
logic out into a class function of Converters, make it an utils.py function or
use it as a function in the converters namespace. 

We chose to put it in the converters.py namespace as a function because it makes
no sense in utils since it is page specific logic, but it is not specific enough
to a given page's instance of its own converter class to be a class function.

I will break this out into its own function in the next patch set if everyone
agrees this makes sense

[Vasily Kuznetsov, 2017/06/23 14:15:24]

As discussed, this approach sounds good.

Now looking at these 3 functions that we're adding to `TemplateConverter` it
starts looking like we should separate all the default globals out into their
own file(s). They are not really part of the converter logic but are more like a
set of services that we provide to the template -- it doesn't seem right to
pollute the converter class with this stuff. The globals often access
`self._params`, which technically is a private attribute of the converter, but
logically that thing is a rendering context and it actually becomes the context
(in jinja sense) of the templates so we will be able to get it using
`contextfunction` decorator. There's also `self._get_locale_data()` that is used
by the globals, but I'm actually wondering if `self._params['localedata']`
should be used instead (it wouldn't load file from the disk the locale every
time and it also supports locale overrides...). I guess we should ask Wladimir
why it's done this way (it's from this change:
https://hg.adblockplus.org/cms/rev/b022896ef69a).

Anyway, you can do the metadata loading refactoring already and perhaps the
separation of the globals will land as a separate change.

[Jon Sonesen, 2017/06/26 07:22:43]

Yeah I totally agree here, and actually we talked about this in the past (not to
this extent detail wise) the fact that we could break out globals and/or filters
out of the converters file tp make it cleaner to extend in the future.

Regarding the locale_data changes I agree here, since instantiating any
converter will override the locale data with user specified parameters. But
maybe there is a side effect we are not considering, or are unaware of.

-        page_metadata = {'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(',')]
-            page_metadata[name.strip()] = value
-        return page_metadata
-
     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']:
+            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]:
@@ skipping 20 matching lines @@
                 stack.pop()
             stack[-1]['subitems'].append(item)
             stack.append(item)
         return structured
 
 converters = {
     'html': RawConverter,
     'md': MarkdownConverter,
     'tmpl': TemplateConverter,
 }