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):

[Vasily Kuznetsov, 2017/07/03 17:42:44]

Perhaps this function should be renamed now since it's actually doing a bit more
than just getting the metadata. I can't think of anything better than something
like `parse_page_content` -- feel free to suggest a better name (we can discuss
on IRC to speed it up).

[juliandoucette, 2017/07/03 21:55:54]

[`get`, `query`, `pages`, `get_pages`, ...]  I like short names.

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

Note that this is not the function that gets exposed to the templates. Also,
what it does is take contents of one page and separate it into metadata and the
rest (which is then used to render the page), so the name should reflect that.

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

Oh, sorry. I meant the get_pages_metadata function (the one that is exposed). It
does more than get pages metadata no (It includes page contents)? So why not
rename it something more appropriate (generic)?

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

Currently the content is not included in metadata and I think it makes sense to
keep it so as far as this ticket is concerned.

P.S. If we include page content, it would be just what's in the file, before the
CMS processing. Not sure if that would be very useful or more confusing.

[juliandoucette, 2017/07/04 10:42:56]

Definitely more confusing.

I don't really care if the content is there (because I haven't had to use it
yet) - but it does logically follow that if the processed content is not in this
object then I should be able to pass this object or the page name to another
function to get it (another ticket ~someday).

-    """Generator which gets per page metadata and cleaned page content"""

[Vasily Kuznetsov, 2017/07/03 17:42:44]

Whenever possible, it's best to write docstrings in the style "Doo foo" or if
we're returning something non-obvious, "Do foo, return bar" or "Do foo, yield
bars" [1]. So perhaps here we could write something like "Separate page content
into metadata (dict) and body text (str)".

[1]: https://www.python.org/dev/peps/pep-0257/#one-line-docstrings

[Jon Sonesen, 2017/07/04 15:02:38]

Acknowledged.

+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]
-        page_data, cleaned_page = get_page_metadata(params['page'], data)

[Vasily Kuznetsov, 2017/07/03 17:42:44]

I think the variable naming is somewhat confusing here. `page_data` should
probably be called `metadata`, since we usually call this metadata elsewhere and
`cleaned_page` could be something like `body_text`. What do you think?

[Jon Sonesen, 2017/07/04 14:58:06]

Agree here, ack

+        page_data, body_text = parse_page_content(params['page'], data)
         params.update(page_data)
-        params[key] = (cleaned_page, filename)
+        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 323 matching lines @@
         )))
 
     def get_pages_metadata(self, filters=None):
         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, cleaned_page = get_page_metadata(page_name, data)

[Vasily Kuznetsov, 2017/07/03 17:42:44]

We can just take the first part of the tuple that `get_page_metadata` returns
and then I would also suggest renaming the variable for clarity. So we'd end up
with:

    metadata = get_page_metadata(...)[0]

What do you think?

[Jon Sonesen, 2017/07/04 14:58:06]

Acknowledged.

+            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']:
             return False
         if filters is None:
@@ skipping 29 matching lines @@
                 stack.pop()
             stack[-1]['subitems'].append(item)
             stack.append(item)
         return structured
 
 converters = {
     'html': RawConverter,
     'md': MarkdownConverter,
     'tmpl': TemplateConverter,
 }