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

Issue #1170 adblockplus.org migration from Anwiki to our CMS.

To test you need to first run the website-conversion script which is available in bitbucket: https://github.com/kzar/adblockplus-website-converter (Read the README.md in that repository for usage instructions.)

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

Only managed to review the first three files today, should be at least something
to keep you going.

http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
File filters/parse_interface.py (right):

http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
filters/parse_interface.py:3: def parse_interface(interfaces):
Nit: I think what it gets as parameter isn't interfaces but rather items inside
an interface.

http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
filters/parse_interface.py:5: for property_key, property in
interfaces.iteritems():
Nit: I think that simple key, value as names would have been better here...

http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
filters/parse_interface.py:6: if property_key.find("(") > -1:
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

http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
filters/parse_interface.py:7: match =
re.match("^(arrayof\W+\w+|\w+)\W+(\w+)[\W\(]+([^\)]*)[\W\)]+$", property_key)
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.

http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
filters/parse_interface.py:9: continue
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?

http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
filters/parse_interface.py:15: property_version = ""
Nit: Use None instead of empty string here?

http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
filters/parse_interface.py:17: for i, argument in
enumerate(argument_string.split(",")):
Nit: Why use enumerate() here? Doesn't seem to be needed.

http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
filters/parse_interface.py:18: parts = argument.split(" ")
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)

http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
filters/parse_interface.py:33: parts = property_key.split()
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.

http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
filters/parse_interface.py:34: property = { }
Nit: no space between the brackets.

http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
filters/parse_interface.py:37: property["modifier"] = parts[0]
I think using temporary variables like you've done it for the methods is a more
readable approach.

http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
filters/parse_interface.py:39: parsed = sorted(parsed, key=lambda x: x["name"])
Please use parse.sort() that will sort the list in-place, without creating a new
one.

http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
File filters/unescape.py (right):

http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
filters/unescape.py:2: return markup.unescape()
You shouldn't need this. If I understand the intention correctly, you can use
the built-in "safe" filter.

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

Cheers for the feedback :)

http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
File filters/parse_interface.py (right):

http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
filters/parse_interface.py:3: def parse_interface(interfaces):
On 2014/09/18 20:33:04, Wladimir Palant wrote:
> Nit: I think what it gets as parameter isn't interfaces but rather items
inside
> an interface.

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
filters/parse_interface.py:5: for property_key, property in
interfaces.iteritems():
On 2014/09/18 20:33:04, Wladimir Palant wrote:
> Nit: I think that simple key, value as names would have been better here...

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
filters/parse_interface.py:6: if property_key.find("(") > -1:
On 2014/09/18 20:33:04, Wladimir Palant wrote:
> 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

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
filters/parse_interface.py:7: match =
re.match("^(arrayof\W+\w+|\w+)\W+(\w+)[\W\(]+([^\)]*)[\W\)]+$", property_key)
On 2014/09/18 20:33:04, Wladimir Palant wrote:
> 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.

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
filters/parse_interface.py:9: continue
On 2014/09/18 20:33:04, Wladimir Palant wrote:
> 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?

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
filters/parse_interface.py:15: property_version = ""
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:
> Nit: Use None instead of empty string here?

http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
filters/parse_interface.py:17: for i, argument in
enumerate(argument_string.split(",")):
On 2014/09/18 20:33:04, Wladimir Palant wrote:
> Nit: Why use enumerate() here? Doesn't seem to be needed.

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
filters/parse_interface.py:18: parts = argument.split(" ")
On 2014/09/18 20:33:04, Wladimir Palant wrote:
> 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)

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
filters/parse_interface.py:33: parts = property_key.split()
On 2014/09/18 20:33:04, Wladimir Palant wrote:
> 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.

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
filters/parse_interface.py:34: property = { }
On 2014/09/18 20:33:04, Wladimir Palant wrote:
> Nit: no space between the brackets.

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
filters/parse_interface.py:37: property["modifier"] = parts[0]
On 2014/09/18 20:33:04, Wladimir Palant wrote:
> I think using temporary variables like you've done it for the methods is a
more
> readable approach.

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
filters/parse_interface.py:39: parsed = sorted(parsed, key=lambda x: x["name"])
On 2014/09/18 20:33:04, Wladimir Palant wrote:
> Please use parse.sort() that will sort the list in-place, without creating a
new
> one.

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
File filters/unescape.py (right):

http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
filters/unescape.py:2: return markup.unescape()
Unfortunately it is required :( For example link tags in descriptions, although
generated by the translate() filter, are then escaped. With just the safe filter
it doesn't work, I found it only worked when I changed translate, unescape and
safe.

On 2014/09/18 20:33:04, Wladimir Palant wrote:
> You shouldn't need this. If I understand the intention correctly, you can use
> the built-in "safe" filter.

[kzar, 2014-09-19 21:08:03]

chained*

On 2014/09/19 11:28:06, kzar wrote:
> Cheers for the feedback :)
> 
>
http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
> File filters/parse_interface.py (right):
> 
>
http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
> filters/parse_interface.py:3: def parse_interface(interfaces):
> On 2014/09/18 20:33:04, Wladimir Palant wrote:
> > Nit: I think what it gets as parameter isn't interfaces but rather items
> inside
> > an interface.
> 
> Done.
> 
>
http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
> filters/parse_interface.py:5: for property_key, property in
> interfaces.iteritems():
> On 2014/09/18 20:33:04, Wladimir Palant wrote:
> > Nit: I think that simple key, value as names would have been better here...
> 
> Done.
> 
>
http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
> filters/parse_interface.py:6: if property_key.find("(") > -1:
> On 2014/09/18 20:33:04, Wladimir Palant wrote:
> > 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
> 
> Done.
> 
>
http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
> filters/parse_interface.py:7: match =
> re.match("^(arrayof\W+\w+|\w+)\W+(\w+)[\W\(]+([^\)]*)[\W\)]+$", property_key)
> On 2014/09/18 20:33:04, Wladimir Palant wrote:
> > 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.
> 
> Done.
> 
>
http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
> filters/parse_interface.py:9: continue
> On 2014/09/18 20:33:04, Wladimir Palant wrote:
> > 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?
> 
> Done.
> 
>
http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
> filters/parse_interface.py:15: property_version = ""
> 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:
> > Nit: Use None instead of empty string here?
> 
>
http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
> filters/parse_interface.py:17: for i, argument in
> enumerate(argument_string.split(",")):
> On 2014/09/18 20:33:04, Wladimir Palant wrote:
> > Nit: Why use enumerate() here? Doesn't seem to be needed.
> 
> Done.
> 
>
http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
> filters/parse_interface.py:18: parts = argument.split(" ")
> On 2014/09/18 20:33:04, Wladimir Palant wrote:
> > 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)
> 
> Done.
> 
>
http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
> filters/parse_interface.py:33: parts = property_key.split()
> On 2014/09/18 20:33:04, Wladimir Palant wrote:
> > 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.
> 
> Done.
> 
>
http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
> filters/parse_interface.py:34: property = { }
> On 2014/09/18 20:33:04, Wladimir Palant wrote:
> > Nit: no space between the brackets.
> 
> Done.
> 
>
http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
> filters/parse_interface.py:37: property["modifier"] = parts[0]
> On 2014/09/18 20:33:04, Wladimir Palant wrote:
> > I think using temporary variables like you've done it for the methods is a
> more
> > readable approach.
> 
> Done.
> 
>
http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
> filters/parse_interface.py:39: parsed = sorted(parsed, key=lambda x:
x["name"])
> On 2014/09/18 20:33:04, Wladimir Palant wrote:
> > Please use parse.sort() that will sort the list in-place, without creating a
> new
> > one.
> 
> Done.
> 
>
http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
> File filters/unescape.py (right):
> 
>
http://codereview.adblockplus.org/5636796054503424/diff/5629499534213120/filt...
> filters/unescape.py:2: return markup.unescape()
> Unfortunately it is required :( For example link tags in descriptions,
although
> generated by the translate() filter, are then escaped. With just the safe
filter
> it doesn't work, I found it only worked when I changed translate, unescape and
> safe.
> 
> On 2014/09/18 20:33:04, Wladimir Palant wrote:
> > You shouldn't need this. If I understand the intention correctly, you can
use
> > the built-in "safe" filter.

[Felix Dahlke, 2014-10-06 12:23:31]

I'll review this soon. Dave, can you add Manvel as a reviewer too? He's the new
Websites module owner.

[kzar, 2014-10-06 12:25:25]

On 2014/10/06 12:23:31, Felix H. Dahlke wrote:
> I'll review this soon. Dave, can you add Manvel as a reviewer too? He's the
new
> Websites module owner.

Thanks and done

[Sebastian Noack, 2015-02-13 17:51:48]

I also realized that some recent changes (like the Maxthon landing page) aren't
included yet.

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/filt...
File filters/parse_interface.py (right):

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/filt...
filters/parse_interface.py:1: import re, warnings
Licence disclaimer is missing, not only here but also in the other files.

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/filt...
filters/parse_interface.py:10: match =
re.match(r"^\s*(%s)\s+(\w+)\s*\(\s*([^\)]*)\s*\)\s*$" % TYPE_REGEXP, key)
Be aware that \w doesn't math dashes and non-ASCII characters. Regardless of
whether this is a real use case here or not, you might just want to match \S
(not a space) instead to reduce code assumptions. Same below.

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/filt...
File filters/subscription_sort.py (right):

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/filt...
filters/subscription_sort.py:1: def subscription_sort(value,
prioritizeRecommended=True):
Nit: Please use underscores instead camelcase in Python, to comply with PEP-8.

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/filt...
filters/subscription_sort.py:8: cmp(b.get("recommendation") != None,
a.get("recommendation") != None) or
None is a singleton. So please always use the "is (not)" operator when
comparing. Or in case you are just checking whether the key exists, use the "in"
operator instead.

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/filt...
filters/subscription_sort.py:9: cmp(a.get("name").lower(),
b.get("name").lower())
This code will raise an exception anyway if "name" doesn't exist. So using
.get() instead canonical item access (square brackets) it pointless here.

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/filt...
File filters/unescape.py (right):

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/filt...
filters/unescape.py:1: def unescape(markup):
As outlined were you use this filter, I don't think that we need it.

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/incl...
File includes/interface.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/incl...
includes/interface.tmpl:4: {{open |safe}}{{ key_name |translate(None,
links[key_name])|unescape|safe }}{{close |safe}}
If you output the "open" and "close" variables outside of the branch you don't
need to duplicate it.

Also from what I can tell the "unescape" and "safe" filters pretty much cancel
out each other here. First you get the raw/unescaped string form a Markup()
object, and than you put it back into another Markup() object.

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/incl...
includes/interface.tmpl:5: {% elif (key_name + i|string) in localedata %}
Maybe using {% set %} instead duplicating the logic to generate the key?

Also I think using string formatting would be more appropriate here, than
converting the number to a string, in order to concatenate the string.

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/incl...
includes/interface.tmpl:86: <p>{{ description(property.name + "Description")
}}</p>
Mind that the "description" macro is already generating a <p> tags.

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/incl...
File includes/preftable.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/incl...
includes/preftable.tmpl:21: <td>{{ preference.default }}</td>
I suppose you should add "colspan=2" here, if you generate two cells otherwise.

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/incl...
File includes/subscriptionList.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/incl...
includes/subscriptionList.tmpl:1: {%- macro processSubscription(subscription,
parent=None) %}
You seem to mix camcelcase with underscore notation. I'd rather stick to
underscore notation for jinja code.

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/incl...
includes/subscriptionList.tmpl:4: <td rowspan="2" class="dummy"></td><td
rowspan="2">
Nit: I think there should be a newline, at least after closing the first cell.

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/incl...
includes/subscriptionList.tmpl:11: <br />{{ "supplements_prefix" |translate }}
{{' / '.join(subscription["supplements"])}} {{ "supplements_suffix" |translate
}}
Assuming this is HTML (not XHTML), there shouldn't be a trailing slash for
self-closing tags, like <br>.

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/incl...
includes/subscriptionList.tmpl:16: {{ "maintainer_prefix" |translate }}
{{subscription["maintainer"]}} {{ "maintainer_suffix" |translate }}<br />
Nit: Any reason you add a space before the pipe indicating a template filter?
This seems to be inconsistent with other jinja2 code we have. I have never seen
this notation before.

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/stat...
File static/css/index.css (right):

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/stat...
static/css/index.css:392: text-align: left;
I suppose the text should be aligned to the read starting position, not
necessarily to the left. Mind right-to-left languages:

text-align: start

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/temp...
File templates/default.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/temp...
templates/default.tmpl:16: <link rel="canonical"
href="https://adblockplus.org/{{not index_page and page or ""}}">
I'd rather use if/else instead and/or here. This makes it more obvious what you
are checking for. And since the else-clause is optional in jinja2 (defaulting to
an empty string), it's also less code:

{{ page if not index_page }}

[Wladimir Palant, 2015-02-13 18:27:56]

On 2015/02/13 17:51:48, Sebastian Noack wrote:
> I also realized that some recent changes (like the Maxthon landing page)
aren't
> included yet.

This shouldn't matter - ideally running the convert.py script
(https://github.com/kzar/adblockplus-website-converter) on the current www
repository state should take care of any changes.

[Wladimir Palant, 2015-02-13 18:31:40]

On 2015/02/13 18:27:56, Wladimir Palant wrote:
> This shouldn't matter - ideally running the convert.py script
> (https://github.com/kzar/adblockplus-website-converter) on the current www
> repository state should take care of any changes.

Sorry, forgot that the Maxthon page is an alias for the index page - this info
cannot be extracted from the repository since it is currently part of the web
server configuration. This means that maxthon.md indeed needs to be created
manually, with <? include index ?> as content. Same goes for the Safari page
btw. (safari.md to replace safari.raw).

[kzar, 2015-02-20 14:55:17]

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/filt...
File filters/parse_interface.py (right):

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/filt...
filters/parse_interface.py:1: import re, warnings
On 2015/02/13 17:51:49, Sebastian Noack wrote:
> Licence disclaimer is missing, not only here but also in the other files.

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/filt...
filters/parse_interface.py:10: match =
re.match(r"^\s*(%s)\s+(\w+)\s*\(\s*([^\)]*)\s*\)\s*$" % TYPE_REGEXP, key)
On 2015/02/13 17:51:49, Sebastian Noack wrote:
> Be aware that \w doesn't math dashes and non-ASCII characters. Regardless of
> whether this is a real use case here or not, you might just want to match \S
> (not a space) instead to reduce code assumptions. Same below.

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/filt...
File filters/subscription_sort.py (right):

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/filt...
filters/subscription_sort.py:1: def subscription_sort(value,
prioritizeRecommended=True):
On 2015/02/13 17:51:49, Sebastian Noack wrote:
> Nit: Please use underscores instead camelcase in Python, to comply with PEP-8.

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/filt...
filters/subscription_sort.py:8: cmp(b.get("recommendation") != None,
a.get("recommendation") != None) or
On 2015/02/13 17:51:49, Sebastian Noack wrote:
> None is a singleton. So please always use the "is (not)" operator when
> comparing. Or in case you are just checking whether the key exists, use the
"in"
> operator instead.

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/filt...
filters/subscription_sort.py:9: cmp(a.get("name").lower(),
b.get("name").lower())
On 2015/02/13 17:51:49, Sebastian Noack wrote:
> This code will raise an exception anyway if "name" doesn't exist. So using
> .get() instead canonical item access (square brackets) it pointless here.

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/filt...
File filters/unescape.py (right):

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/filt...
filters/unescape.py:1: def unescape(markup):
On 2015/02/13 17:51:49, Sebastian Noack wrote:
> As outlined were you use this filter, I don't think that we need it.

Unfortunately we do.

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/incl...
File includes/interface.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/incl...
includes/interface.tmpl:4: {{open |safe}}{{ key_name |translate(None,
links[key_name])|unescape|safe }}{{close |safe}}
On 2015/02/13 17:51:49, Sebastian Noack wrote:
> If you output the "open" and "close" variables outside of the branch you don't
> need to duplicate it.
> 
> Also from what I can tell the "unescape" and "safe" filters pretty much cancel
> out each other here. First you get the raw/unescaped string form a Markup()
> object, and than you put it back into another Markup() object.

Both the unescape and safe filters are required here, I can't remember exactly
why. I just tested and some interface descriptions broke without them.

I have moved output of open and close like suggested and also I found that the
unescape + safe filters were not required for the return description so I
removed them there.

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/incl...
includes/interface.tmpl:5: {% elif (key_name + i|string) in localedata %}
On 2015/02/13 17:51:49, Sebastian Noack wrote:
> Maybe using {% set %} instead duplicating the logic to generate the key?
> 
> Also I think using string formatting would be more appropriate here, than
> converting the number to a string, in order to concatenate the string.

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/incl...
includes/interface.tmpl:86: <p>{{ description(property.name + "Description")
}}</p>
On 2015/02/13 17:51:49, Sebastian Noack wrote:
> Mind that the "description" macro is already generating a <p> tags. 

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/incl...
File includes/preftable.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/incl...
includes/preftable.tmpl:21: <td>{{ preference.default }}</td>
On 2015/02/13 17:51:49, Sebastian Noack wrote:
> I suppose you should add "colspan=2" here, if you generate two cells
otherwise.

I only generate one cell otherwise I think.

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/incl...
File includes/subscriptionList.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/incl...
includes/subscriptionList.tmpl:1: {%- macro processSubscription(subscription,
parent=None) %}
On 2015/02/13 17:51:49, Sebastian Noack wrote:
> You seem to mix camcelcase with underscore notation. I'd rather stick to
> underscore notation for jinja code.

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/incl...
includes/subscriptionList.tmpl:4: <td rowspan="2" class="dummy"></td><td
rowspan="2">
On 2015/02/13 17:51:49, Sebastian Noack wrote:
> Nit: I think there should be a newline, at least after closing the first cell.

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/incl...
includes/subscriptionList.tmpl:11: <br />{{ "supplements_prefix" |translate }}
{{' / '.join(subscription["supplements"])}} {{ "supplements_suffix" |translate
}}
On 2015/02/13 17:51:49, Sebastian Noack wrote:
> Assuming this is HTML (not XHTML), there shouldn't be a trailing slash for
> self-closing tags, like <br>.

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/incl...
includes/subscriptionList.tmpl:16: {{ "maintainer_prefix" |translate }}
{{subscription["maintainer"]}} {{ "maintainer_suffix" |translate }}<br />
On 2015/02/13 17:51:49, Sebastian Noack wrote:
> Nit: Any reason you add a space before the pipe indicating a template filter?
> This seems to be inconsistent with other jinja2 code we have. I have never
seen
> this notation before.

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/stat...
File static/css/index.css (right):

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/stat...
static/css/index.css:392: text-align: left;
On 2015/02/13 17:51:49, Sebastian Noack wrote:
> I suppose the text should be aligned to the read starting position, not
> necessarily to the left. Mind right-to-left languages:
> 
> text-align: start

CSS and other static files like images are imported from the live site. Fixes
need to be made to the live site currently and then imported here.

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/temp...
File templates/default.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/temp...
templates/default.tmpl:16: <link rel="canonical"
href="https://adblockplus.org/{{not index_page and page or ""}}">
On 2015/02/13 17:51:49, Sebastian Noack wrote:
> I'd rather use if/else instead and/or here. This makes it more obvious what
you
> are checking for. And since the else-clause is optional in jinja2 (defaulting
to
> an empty string), it's also less code:
> 
> {{ page if not index_page }}

Done.

[Sebastian Noack, 2015-02-20 15:32:59]

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/incl...
File includes/interface.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/incl...
includes/interface.tmpl:4: {{open |safe}}{{ key_name |translate(None,
links[key_name])|unescape|safe }}{{close |safe}}
On 2015/02/20 14:55:18, kzar wrote:
> On 2015/02/13 17:51:49, Sebastian Noack wrote:
> > If you output the "open" and "close" variables outside of the branch you
don't
> > need to duplicate it.
> > 
> > Also from what I can tell the "unescape" and "safe" filters pretty much
cancel
> > out each other here. First you get the raw/unescaped string form a Markup()
> > object, and than you put it back into another Markup() object.
> 
> Both the unescape and safe filters are required here, I can't remember exactly
> why. I just tested and some interface descriptions broke without them.

I think I figured out the issue here. It look like there are some translations
in the form "<foo>". And this hack removes the escaping. However, we
should rather fix the translations, than applying a hack here.

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/incl...
File includes/preftable.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/incl...
includes/preftable.tmpl:21: <td>{{ preference.default }}</td>
On 2015/02/20 14:55:18, kzar wrote:
> On 2015/02/13 17:51:49, Sebastian Noack wrote:
> > I suppose you should add "colspan=2" here, if you generate two cells
> otherwise.
> 
> I only generate one cell otherwise I think.

Oops, you are right.

http://codereview.adblockplus.org/5636796054503424/diff/5643137129119744/stat...
File static/css/index-desktop.css (right):

http://codereview.adblockplus.org/5636796054503424/diff/5643137129119744/stat...
static/css/index-desktop.css:52: margin-left: 18px;
Not that great for right-to-left languages, but given that this also needs to
render in IE 8+, considering right-to-left languages here would be rather ugly.

[kzar, 2015-02-20 17:04:47]

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/filt...
File filters/unescape.py (right):

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/filt...
filters/unescape.py:1: def unescape(markup):
On 2015/02/20 14:55:18, kzar wrote:
> On 2015/02/13 17:51:49, Sebastian Noack wrote:
> > As outlined were you use this filter, I don't think that we need it.
> 
> Unfortunately we do.

So I've just checked and there appear to be no locale strings with escaped HTML
saved in the locale files. There are however a couple of places[1] where we need
more HTML than is allowed by sitescripts.cms, sitescripts.cms escapes the HTML
and we then have to unescape it again before it's displayed.

Ultimately you're right we should remove the need for this, but I would prefer
to split that work off as a separate ticket and merge this first.

[1] Unescape filter is used in two places; for interface and preference
descriptions. Of those I can find in total 10 examples where the filter makes a
difference.

http://codereview.adblockplus.org/5636796054503424/diff/5643137129119744/stat...
File static/css/index-desktop.css (right):

http://codereview.adblockplus.org/5636796054503424/diff/5643137129119744/stat...
static/css/index-desktop.css:52: margin-left: 18px;
On 2015/02/20 15:32:59, Sebastian Noack wrote:
> Not that great for right-to-left languages, but given that this also needs to
> render in IE 8+, considering right-to-left languages here would be rather
ugly.

Again, no need to review the CSS here as it's just being brought over from the
live site.

[Sebastian Noack, 2015-02-20 17:49:38]

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/filt...
File filters/unescape.py (right):

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/filt...
filters/unescape.py:1: def unescape(markup):
On 2015/02/20 17:04:48, kzar wrote:
> On 2015/02/20 14:55:18, kzar wrote:
> > On 2015/02/13 17:51:49, Sebastian Noack wrote:
> > > As outlined were you use this filter, I don't think that we need it.
> > 
> > Unfortunately we do.
> 
> So I've just checked and there appear to be no locale strings with escaped
HTML
> saved in the locale files. There are however a couple of places[1] where we
need
> more HTML than is allowed by sitescripts.cms, sitescripts.cms escapes the HTML
> and we then have to unescape it again before it's displayed.
> 
> Ultimately you're right we should remove the need for this, but I would prefer
> to split that work off as a separate ticket and merge this first.
> 
> [1] Unescape filter is used in two places; for interface and preference
> descriptions. Of those I can find in total 10 examples where the filter makes
a
> difference.

You are right, the "translate" template filter escapes everything except <a>,
<strong> and <em> tags. I missed that, makes sense though. But if this isn't
enough, you have to change that implementation first.

We are not talking about an aesthetic issue here. You are potentially breaking
the website here. For example, imagine following string: "reviewer > author".
The "translate" filter will correctly turn it into "reviewer &gt; author". But
then you turn it back into "reviewer > author" breaking the generated HTML.

Not to mention that you enable translators to perform XSS attacks.

[kzar, 2015-02-20 18:00:43]

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/filt...
File filters/unescape.py (right):

http://codereview.adblockplus.org/5636796054503424/diff/5730192894984192/filt...
filters/unescape.py:1: def unescape(markup):
On 2015/02/20 17:49:38, Sebastian Noack wrote:
> On 2015/02/20 17:04:48, kzar wrote:
> > On 2015/02/20 14:55:18, kzar wrote:
> > > On 2015/02/13 17:51:49, Sebastian Noack wrote:
> > > > As outlined were you use this filter, I don't think that we need it.
> > > 
> > > Unfortunately we do.
> > 
> > So I've just checked and there appear to be no locale strings with escaped
> HTML
> > saved in the locale files. There are however a couple of places[1] where we
> need
> > more HTML than is allowed by sitescripts.cms, sitescripts.cms escapes the
HTML
> > and we then have to unescape it again before it's displayed.
> > 
> > Ultimately you're right we should remove the need for this, but I would
prefer
> > to split that work off as a separate ticket and merge this first.
> > 
> > [1] Unescape filter is used in two places; for interface and preference
> > descriptions. Of those I can find in total 10 examples where the filter
makes
> a
> > difference.
> 
> You are right, the "translate" template filter escapes everything except <a>,
> <strong> and <em> tags. I missed that, makes sense though. But if this isn't
> enough, you have to change that implementation first.
> 
> We are not talking about an aesthetic issue here. You are potentially breaking
> the website here. For example, imagine following string: "reviewer > author".
> The "translate" filter will correctly turn it into "reviewer &gt; author". But
> then you turn it back into "reviewer > author" breaking the generated HTML.
> 
> Not to mention that you enable translators to perform XSS attacks.

Well in for these two places there are sometimes descriptions that contain
links. Normally we can safely include links by only having the <a> tag itself in
the locale string and then passing the URLs as paramers from the page template.
But for preference and interface descriptions I don't think this is possible.

This is because we are using the preftable and interface includes for these
pages, to avoid having the link URLs in the locale strings we would need to
somehow either put them in the includes directly or in the pages that include
them.

So unless you can see another way to approach this I think we have the choice
between using this unescape filter and modifying the sitescripts.cms code to
allow <a> tags with the href attribute.

What do you think?

[kzar, 2015-02-23 19:01:58]

OK so it looks like I already solved the problem of passing links into the
descriptions, a dictionary of link arrays is already generated and set in the
pages. All I had to do was strip the href attributes in the strings so that the
code could do it's job - now everything seems to work fine without the unescape
filter!

It looks like I just forgot to strip the href attributes before and that the
unescaping was no longer necessary! Sorry!

[Sebastian Noack, 2015-02-23 19:51:10]

LGTM

[Wladimir Palant, 2015-02-24 20:19:40]

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/filt...
File filters/parse_interface.py (right):

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/filt...
filters/parse_interface.py:34: property_version = ""
Nit: how about:

  property_version = value.get("version", "")

Or just skip creating that variable and use value.get() in parsed.append()
below.

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/filt...
File filters/subscription_sort.py (right):

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/filt...
filters/subscription_sort.py:25: ))
Nit: You can use the global sorted() function, then you won't need to create a
copy of the list:

  return sorted(value, lambda a, b: ...)

And - yes, I know that you simply copied my own code. But I wrote that code
years ago back when I knew very little about Python.

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
File includes/index.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
includes/index.tmpl:17: noheading=True
Why is your base file different from index.tmpl currently in the wwwnew
repository? Yours doesn't set localefile attribute.

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
File includes/interface.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
includes/interface.tmpl:18: {% macro description(key_name, open="<p>",
close="</p>") %}
Nit: Looking at how this macro is used, having open="", close="" as default
values seems to make more sense. <p> and </p> can be specified explicitly in the
one case separators are required.

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
includes/interface.tmpl:24: {%- endif %}
Nit: Why the space trimming here? We don't do it elsewhere.

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
includes/interface.tmpl:26: {{open|safe}}{{ current_key_name|translate(None,
links[key_name]) }}{{close|safe}}
Why are links a separate variable rather than part of the interface definition?
Method/property declarations should simply have a description_links property.

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
includes/interface.tmpl:34: {{ return_type -}}
Nit: Again, why the space trimming here? It should have no impact on the way
things are displayed.

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
includes/interface.tmpl:42: {% set interface=interface|parse_interface %}
I seriously dislike how this passes parameters via global variables. How about
using template inheritance instead. So an interface file would do:

{% extends "includes/interface.tmpl" %}

{{ interface([
  ...
]) }}

So instead of a block body here there would be a macro interface() that could
take parameters properly.

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
includes/interface.tmpl:44: <h2>{{ "methods_and_properties"|translate }}</h2>
Nit: Call this string "toc_header"? The string identifier should reflect what it
is used for, not what its current value is.

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
includes/interface.tmpl:69: <hr>
Nit: Move <hr> out of the if block? We need a separator regardless of whether
this is a method or property.

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
includes/interface.tmpl:84: <dd>{{ property.version }} and higher</dd>
The two strings above should be translated.

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
includes/interface.tmpl:87: <dt>Arguments:</dt>
This string should be translated.

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
includes/interface.tmpl:91: {{ description(property.name + "Argument" +
argument.name + "Description", "", "") }}
"updateExternalSubscriptionArgumenttitleDescription" is a rather messy string
and unreadable string identifier. How about using
"updateExternalSubscription-titleDescription" instead?

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
includes/interface.tmpl:96: <dt>Returns:</dt>
This string should be translated.

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
includes/interface.tmpl:97: <dd>{{ (property.name +
"ReturnDescription")|translate(None, links[property.name + "ReturnDescription"])
}}</dd>
Why not use the description() macro here? You did it for arguments which are
pretty similar.

Again, how about "updateExternalSubscription-returnDescription" as string name,
for readability? Note that there cannot be a parameter called "return" - it's a
reserved keyword.

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
File includes/preftable.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
includes/preftable.tmpl:18: {% block body %}
Here as well, could you use template inheritance and proper parameter passing?

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
includes/preftable.tmpl:28: <td class="section" colspan="3">{{
section.title|translate }}</td>
Use section.id + "SectionTitle" instead of section.title, for consistency with
the interface template?

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
includes/preftable.tmpl:40: {% if preference.empty != "false" %}
The empty field is a hack because the preference value can only be a string in
Anwiki - so distinguishing "" and empty is hard. In Python (or Jinja2), the
latter can be indicated by None, without any additional flag.

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
includes/preftable.tmpl:46: <td>{{ preference.description|translate(None,
links[preference.description]) }}</td>
Please allow for multiple description paragraphs, as you did with interfaces.
The fact that I used <br> to separate paragraphs in Anwiki was a hack.

Also, links shouldn't be a separate variable but rather part of the preference:
preference.links rather than links[preference.description].

Nit: Specifying the parameter by name should work better here:

  preference.description|translate(links=preference.links)

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
File includes/subscriptionList.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
includes/subscriptionList.tmpl:74: {%- for subscription in
subscriptions|subscription_sort -%}
As with other templates, please use template inheritance and explicit parameter
passing here.

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
File includes/subscriptions.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
includes/subscriptions.tmpl:18: {% set subscriptions = [
This definitely shouldn't be a hardcoded list. How about defining a
pseudo-filter instead that will download the subscriptionlist repository and
parse the subscriptions out of it?

import codecs
import tarfile
import urllib

from sitescripts.subscriptions.subscriptionParser import parse_file,
calculate_supplemented

def get_subscriptions(dummy):
  result = {}
  source =
urllib.urlopen("https://hg.adblockplus.org/subscriptionlist/archive/default.tar.gz")
  with tarfile.open(fileobj=source, mode="r|gz") as archive:
    for fileinfo in archive:
      if not fileinfo.name.endswith(".subscription"):
        continue

      filedata = parse_file(fileinfo.name,
codecs.getreader('utf8')(archive.extractfile(fileinfo)))
      if filedata.unavailable:
        continue

      result[filedata.name] = filedata

  calculate_supplemented(result)
  return result

Unfortunately, this duplicates some logic from
subscriptionParser.readSubscriptions() - this can be fixed later, by making that
function accept arbitrary sources.

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/temp...
File templates/default.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/temp...
templates/default.tmpl:33: <link rel="canonical"
href="https://adblockplus.org/{{page if not index_page}}">
No, we should have https://adblockplus.org/firefox and
https://adblockplus.org/chrome as canonical links - this is different from our
current website as the content of these pages can be different now.

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/temp...
templates/default.tmpl:67: {% if index_page %}
Where is that variable being defined?

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/temp...
templates/default.tmpl:105: <div id="content" class="{{page}}">
Please remove that class tag - this can easily turn into a footgun if we
accidentally name a page like a class we are using.

[kzar, 2015-02-26 19:50:16]

Addressed palant's feedback.

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/filt...
File filters/parse_interface.py (right):

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/filt...
filters/parse_interface.py:34: property_version = ""
On 2015/02/24 20:19:40, Wladimir Palant wrote:
> Nit: how about:
> 
>   property_version = value.get("version", "")
> 
> Or just skip creating that variable and use value.get() in parsed.append()
> below.

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/filt...
File filters/subscription_sort.py (right):

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/filt...
filters/subscription_sort.py:25: ))
On 2015/02/24 20:19:40, Wladimir Palant wrote:
> Nit: You can use the global sorted() function, then you won't need to create a
> copy of the list:
> 
>   return sorted(value, lambda a, b: ...)
> 
> And - yes, I know that you simply copied my own code. But I wrote that code
> years ago back when I knew very little about Python.

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
File includes/index.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
includes/index.tmpl:17: noheading=True
On 2015/02/24 20:19:40, Wladimir Palant wrote:
> Why is your base file different from index.tmpl currently in the wwwnew
> repository? Yours doesn't set localefile attribute.

I think that is because the change is made by the conversion script and I go to
some lengths to only include manual changes in this review. (I create a
temporary git branch, commit all my changes, create another temporary branch and
then re-run the conversion script and then finally pull in any changes that are
unique to the first branch.)

https://github.com/kzar/adblockplus-website-converter/commit/e3bb12319f1a7eb7...

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
File includes/interface.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
includes/interface.tmpl:18: {% macro description(key_name, open="<p>",
close="</p>") %}
On 2015/02/24 20:19:40, Wladimir Palant wrote:
> Nit: Looking at how this macro is used, having open="", close="" as default
> values seems to make more sense. <p> and </p> can be specified explicitly in
the
> one case separators are required.

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
includes/interface.tmpl:24: {%- endif %}
On 2015/02/24 20:19:40, Wladimir Palant wrote:
> Nit: Why the space trimming here? We don't do it elsewhere.

Since I refactored this code the trimming no longer makes sense. I removed most
of them but must have missed this one, removed.

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
includes/interface.tmpl:26: {{open|safe}}{{ current_key_name|translate(None,
links[key_name]) }}{{close|safe}}
On 2015/02/24 20:19:40, Wladimir Palant wrote:
> Why are links a separate variable rather than part of the interface
definition?
> Method/property declarations should simply have a description_links property.

Well I think because it's not that simple. Interfaces can have descriptions,
return descriptions and descriptions for each of their arguments. Then for each
of those descriptions you can have multiple paragraphs which you need to
differentiate between as well.

This is possible but I'd really rather not go there, especially because it will
complicated the conversion script quite a lot as well. (Worth noting with this
that there are relatively few links inside descriptions in practice. Adjusting
the data structure in the preference and two interface templates later would be
way easier than messing with the conversion script at this stage.)

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
includes/interface.tmpl:34: {{ return_type -}}
On 2015/02/24 20:19:40, Wladimir Palant wrote:
> Nit: Again, why the space trimming here? It should have no impact on the way
> things are displayed.

This one is required, otherwise the link has a trailing space causing the space
in between the return type and interface to be part of the link.

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
includes/interface.tmpl:42: {% set interface=interface|parse_interface %}
On 2015/02/24 20:19:40, Wladimir Palant wrote:
> I seriously dislike how this passes parameters via global variables. How about
> using template inheritance instead. So an interface file would do:
> 
> {% extends "includes/interface.tmpl" %}
> 
> {{ interface([
>   ...
> ]) }}
> 
> So instead of a block body here there would be a macro interface() that could
> take parameters properly.

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
includes/interface.tmpl:44: <h2>{{ "methods_and_properties"|translate }}</h2>
On 2015/02/24 20:19:40, Wladimir Palant wrote:
> Nit: Call this string "toc_header"? The string identifier should reflect what
it
> is used for, not what its current value is.

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
includes/interface.tmpl:69: <hr>
On 2015/02/24 20:19:40, Wladimir Palant wrote:
> Nit: Move <hr> out of the if block? We need a separator regardless of whether
> this is a method or property.

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
includes/interface.tmpl:84: <dd>{{ property.version }} and higher</dd>
On 2015/02/24 20:19:40, Wladimir Palant wrote:
> The two strings above should be translated.

I agree but it's not currently and this issue is to migrate the existing
content. Shouldn't these translations be a separate issue? (I created a meta
issue for the website migration we can put things like this under
https://issues.adblockplus.org/ticket/2035 )

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
includes/interface.tmpl:87: <dt>Arguments:</dt>
On 2015/02/24 20:19:40, Wladimir Palant wrote:
> This string should be translated.

See above.

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
includes/interface.tmpl:91: {{ description(property.name + "Argument" +
argument.name + "Description", "", "") }}
On 2015/02/24 20:19:40, Wladimir Palant wrote:
> "updateExternalSubscriptionArgumenttitleDescription" is a rather messy string
> and unreadable string identifier. How about using
> "updateExternalSubscription-titleDescription" instead?

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
includes/interface.tmpl:96: <dt>Returns:</dt>
On 2015/02/24 20:19:40, Wladimir Palant wrote:
> This string should be translated.

See above.

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
includes/interface.tmpl:97: <dd>{{ (property.name +
"ReturnDescription")|translate(None, links[property.name + "ReturnDescription"])
}}</dd>
On 2015/02/24 20:19:40, Wladimir Palant wrote:
> Why not use the description() macro here? You did it for arguments which are
> pretty similar.
> 
> Again, how about "updateExternalSubscription-returnDescription" as string
name,
> for readability? Note that there cannot be a parameter called "return" - it's
a
> reserved keyword.

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
File includes/preftable.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
includes/preftable.tmpl:18: {% block body %}
On 2015/02/24 20:19:40, Wladimir Palant wrote:
> Here as well, could you use template inheritance and proper parameter passing?

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
includes/preftable.tmpl:28: <td class="section" colspan="3">{{
section.title|translate }}</td>
On 2015/02/24 20:19:40, Wladimir Palant wrote:
> Use section.id + "SectionTitle" instead of section.title, for consistency with
> the interface template?

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
includes/preftable.tmpl:40: {% if preference.empty != "false" %}
On 2015/02/24 20:19:40, Wladimir Palant wrote:
> The empty field is a hack because the preference value can only be a string in
> Anwiki - so distinguishing "" and empty is hard. In Python (or Jinja2), the
> latter can be indicated by None, without any additional flag.

So the logic here around preference.empty seemed broken, I've improved it now so
that the output matches the live site.

I don't think I understand your comment though, looking at the generated
preferences.tmpl I see some preferences with the default of "" and empty of
"false" and some with default of "" with empty of "true".

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
includes/preftable.tmpl:46: <td>{{ preference.description|translate(None,
links[preference.description]) }}</td>
On 2015/02/24 20:19:40, Wladimir Palant wrote:
> Please allow for multiple description paragraphs, as you did with interfaces.
> The fact that I used <br> to separate paragraphs in Anwiki was a hack.
> 
> Also, links shouldn't be a separate variable but rather part of the
preference:
> preference.links rather than links[preference.description].
> 
> Nit: Specifying the parameter by name should work better here:
> 
>   preference.description|translate(links=preference.links)

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
File includes/subscriptionList.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
includes/subscriptionList.tmpl:74: {%- for subscription in
subscriptions|subscription_sort -%}
On 2015/02/24 20:19:40, Wladimir Palant wrote:
> As with other templates, please use template inheritance and explicit
parameter
> passing here.

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
File includes/subscriptions.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
includes/subscriptions.tmpl:18: {% set subscriptions = [
On 2015/02/24 20:19:40, Wladimir Palant wrote:
> This definitely shouldn't be a hardcoded list. How about defining a
> pseudo-filter instead that will download the subscriptionlist repository and
> parse the subscriptions out of it?
> 
> import codecs
> import tarfile
> import urllib
> 
> from sitescripts.subscriptions.subscriptionParser import parse_file,
> calculate_supplemented
> 
> def get_subscriptions(dummy):
>   result = {}
>   source =
>
urllib.urlopen("https://hg.adblockplus.org/subscriptionlist/archive/default.tar.gz")
>   with tarfile.open(fileobj=source, mode="r|gz") as archive:
>     for fileinfo in archive:
>       if not fileinfo.name.endswith(".subscription"):
>         continue
> 
>       filedata = parse_file(fileinfo.name,
> codecs.getreader('utf8')(archive.extractfile(fileinfo)))
>       if filedata.unavailable:
>         continue
> 
>       result[filedata.name] = filedata
> 
>   calculate_supplemented(result)
>   return result
> 
> Unfortunately, this duplicates some logic from
> subscriptionParser.readSubscriptions() - this can be fixed later, by making
that
> function accept arbitrary sources.

I'm pretty sure you said to do it this way on the issue
https://issues.adblockplus.org/ticket/1170#comment:9 . Maybe I misunderstood,
either way this is much cooler :D

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/temp...
File templates/default.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/temp...
templates/default.tmpl:33: <link rel="canonical"
href="https://adblockplus.org/{{page if not index_page}}">
On 2015/02/24 20:19:40, Wladimir Palant wrote:
> No, we should have https://adblockplus.org/firefox and
> https://adblockplus.org/chrome as canonical links - this is different from our
> current website as the content of these pages can be different now.

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/temp...
templates/default.tmpl:67: {% if index_page %}
On 2015/02/24 20:19:40, Wladimir Palant wrote:
> Where is that variable being defined?

At the top of this page.

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/temp...
templates/default.tmpl:105: <div id="content" class="{{page}}">
On 2015/02/24 20:19:40, Wladimir Palant wrote:
> Please remove that class tag - this can easily turn into a footgun if we
> accidentally name a page like a class we are using.

The current site adds a class like that to the div, I tried removing it just now
and some of the layout breaks. Best I can do is only add the class for index
pages, that OK? (Actually that is a closer emulation of how the current site
works.)

[Wladimir Palant, 2015-02-26 20:35:03]

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
File includes/interface.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
includes/interface.tmpl:26: {{open|safe}}{{ current_key_name|translate(None,
links[key_name]) }}{{close|safe}}
On 2015/02/26 19:50:17, kzar wrote:
> Well I think because it's not that simple. Interfaces can have descriptions,
> return descriptions and descriptions for each of their arguments. Then for
each
> of those descriptions you can have multiple paragraphs which you need to
> differentiate between as well.

Fair enough. Having links in a separate data structure makes editing harder but
I see how this is complicated right now.

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
includes/interface.tmpl:84: <dd>{{ property.version }} and higher</dd>
On 2015/02/26 19:50:17, kzar wrote:
> I agree but it's not currently and this issue is to migrate the existing
> content. Shouldn't these translations be a separate issue?

Heh, fair enough.

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
File includes/preftable.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
includes/preftable.tmpl:40: {% if preference.empty != "false" %}
On 2015/02/26 19:50:17, kzar wrote:
> I don't think I understand your comment though, looking at the generated
> preferences.tmpl I see some preferences with the default of "" and empty of
> "false" and some with default of "" with empty of "true".

The ones where empty is "true" should have None as value instead - that's enough
to distinguish preferences where the default value is empty. The ones where the
default is "" which aren't "empty" are merely values we don't list - typically
URLs that would be pointless in that table.

So instead of testing preference.empty != "false" you would test
preference.value is None.

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/temp...
File templates/default.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/temp...
templates/default.tmpl:105: <div id="content" class="{{page}}">
On 2015/02/26 19:50:17, kzar wrote:
> The current site adds a class like that to the div,

I see. Setting this on the content tag is a limitation of our current setup - I
moved that class to the install-container element in includes/index.tmpl.
However, that approach requires CSS changes so I am fine with keeping this hack
for now.

http://codereview.adblockplus.org/5636796054503424/diff/5695159920492544/filt...
File filters/get_subscriptions.py (right):

http://codereview.adblockplus.org/5636796054503424/diff/5695159920492544/filt...
filters/get_subscriptions.py:16: import codecs, tarfile, urllib
Nit: One import per line please, as required by PEP8 (yes, our old code doesn't
do that).

http://codereview.adblockplus.org/5636796054503424/diff/5695159920492544/filt...
filters/get_subscriptions.py:25: return d
Why is this necessary? The original template in the sitescripts repository
operates directly on the Subscription objects.

Accessing private properties (_data) is definitely a very bad idea.

http://codereview.adblockplus.org/5636796054503424/diff/5695159920492544/incl...
File includes/interface.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5695159920492544/incl...
includes/interface.tmpl:85: {{ description(property.name + "-returnDescription",
links, "<dd>", "</dd>") }}
No, you don't want to create another <dd> block for each paragraph. It should
really be <dd>{{description(...)}}</dd>

http://codereview.adblockplus.org/5636796054503424/diff/5695159920492544/incl...
File includes/preftable.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5695159920492544/incl...
includes/preftable.tmpl:41: <td>empty</td>
Nit: Currently it's <td><em>empty</em></td>

http://codereview.adblockplus.org/5636796054503424/diff/5695159920492544/incl...
includes/preftable.tmpl:45: <td>{{ description(preference.description, links,
"", "<br>") }}</td>
Didn't realize that the string ID is part of the data structure again -
preference.name + "Description" for consistency maybe?

http://codereview.adblockplus.org/5636796054503424/diff/5695159920492544/temp...
File templates/default.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5695159920492544/temp...
templates/default.tmpl:18: {% set index_page = (pagedata == "<? include index
?>\n") %}
Please use (localefile == "index") here, as it was done originally. pagedata is
way too unreliable, what if somebody forgets that linebreak at the end or adds
one more?

[kzar, 2015-02-26 21:49:13]

Addressed more comments.

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
File includes/interface.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
includes/interface.tmpl:84: <dd>{{ property.version }} and higher</dd>
On 2015/02/26 20:35:03, Wladimir Palant wrote:
> On 2015/02/26 19:50:17, kzar wrote:
> > I agree but it's not currently and this issue is to migrate the existing
> > content. Shouldn't these translations be a separate issue?
> 
> Heh, fair enough.

Cool, I've filed an issue https://issues.adblockplus.org/ticket/2054

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
File includes/preftable.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5667766619078656/incl...
includes/preftable.tmpl:40: {% if preference.empty != "false" %}
On 2015/02/26 20:35:03, Wladimir Palant wrote:
> On 2015/02/26 19:50:17, kzar wrote:
> > I don't think I understand your comment though, looking at the generated
> > preferences.tmpl I see some preferences with the default of "" and empty of
> > "false" and some with default of "" with empty of "true".
> 
> The ones where empty is "true" should have None as value instead - that's
enough
> to distinguish preferences where the default value is empty. The ones where
the
> default is "" which aren't "empty" are merely values we don't list - typically
> URLs that would be pointless in that table.
> 
> So instead of testing preference.empty != "false" you would test
> preference.value is None.

Ah I see, done that now. One annoyance was that the website converter is
changing None to null, so the comparison has to be `if preference.default is not
string` instead of just `if preference.default is none` but nevermind.

http://codereview.adblockplus.org/5636796054503424/diff/5695159920492544/filt...
File filters/get_subscriptions.py (right):

http://codereview.adblockplus.org/5636796054503424/diff/5695159920492544/filt...
filters/get_subscriptions.py:16: import codecs, tarfile, urllib
On 2015/02/26 20:35:03, Wladimir Palant wrote:
> Nit: One import per line please, as required by PEP8 (yes, our old code
doesn't
> do that).

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5695159920492544/filt...
filters/get_subscriptions.py:25: return d
On 2015/02/26 20:35:03, Wladimir Palant wrote:
> Why is this necessary? The original template in the sitescripts repository
> operates directly on the Subscription objects.
> 
> Accessing private properties (_data) is definitely a very bad idea.

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5695159920492544/incl...
File includes/interface.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5695159920492544/incl...
includes/interface.tmpl:85: {{ description(property.name + "-returnDescription",
links, "<dd>", "</dd>") }}
On 2015/02/26 20:35:03, Wladimir Palant wrote:
> No, you don't want to create another <dd> block for each paragraph. It should
> really be <dd>{{description(...)}}</dd>

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5695159920492544/incl...
File includes/preftable.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5695159920492544/incl...
includes/preftable.tmpl:41: <td>empty</td>
On 2015/02/26 20:35:03, Wladimir Palant wrote:
> Nit: Currently it's <td><em>empty</em></td>

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5695159920492544/incl...
includes/preftable.tmpl:45: <td>{{ description(preference.description, links,
"", "<br>") }}</td>
On 2015/02/26 20:35:03, Wladimir Palant wrote:
> Didn't realize that the string ID is part of the data structure again -
> preference.name + "Description" for consistency maybe?

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5695159920492544/temp...
File templates/default.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5695159920492544/temp...
templates/default.tmpl:18: {% set index_page = (pagedata == "<? include index
?>\n") %}
On 2015/02/26 20:35:03, Wladimir Palant wrote:
> Please use (localefile == "index") here, as it was done originally. pagedata
is
> way too unreliable, what if somebody forgets that linebreak at the end or adds
> one more?

Done.

[Wladimir Palant, 2015-02-26 22:01:46]

http://codereview.adblockplus.org/5636796054503424/diff/5164320753188864/filt...
File filters/get_subscriptions.py (right):

http://codereview.adblockplus.org/5636796054503424/diff/5164320753188864/filt...
filters/get_subscriptions.py:37: return list(result.itervalues())
return result.values()?

http://codereview.adblockplus.org/5636796054503424/diff/5164320753188864/filt...
File filters/subscription_sort.py (right):

http://codereview.adblockplus.org/5636796054503424/diff/5164320753188864/filt...
filters/subscription_sort.py:19: cmp(getattr(a, "type", None), getattr(b,
"type", None)) or
Why not cmp(a.type, b.type) like in the original function? Same for the other
attributes of course. All these attributes are guaranteed to exist.

http://codereview.adblockplus.org/5636796054503424/diff/5164320753188864/incl...
File includes/preftable.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5164320753188864/incl...
includes/preftable.tmpl:40: {% elif preference.default is not string %}
preference.default is none? We might use actual numbers in preference.default...

[kzar, 2015-02-26 22:12:16]

Addressed a few more comments.

http://codereview.adblockplus.org/5636796054503424/diff/5164320753188864/filt...
File filters/get_subscriptions.py (right):

http://codereview.adblockplus.org/5636796054503424/diff/5164320753188864/filt...
filters/get_subscriptions.py:37: return list(result.itervalues())
On 2015/02/26 22:01:46, Wladimir Palant wrote:
> return result.values()?

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5164320753188864/filt...
File filters/subscription_sort.py (right):

http://codereview.adblockplus.org/5636796054503424/diff/5164320753188864/filt...
filters/subscription_sort.py:19: cmp(getattr(a, "type", None), getattr(b,
"type", None)) or
On 2015/02/26 22:01:46, Wladimir Palant wrote:
> Why not cmp(a.type, b.type) like in the original function? Same for the other
> attributes of course. All these attributes are guaranteed to exist.

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5164320753188864/incl...
File includes/preftable.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5164320753188864/incl...
includes/preftable.tmpl:40: {% elif preference.default is not string %}
On 2015/02/26 22:01:46, Wladimir Palant wrote:
> preference.default is none? We might use actual numbers in
preference.default...

I did this because the conversion script annoyingly converts None to null. How
about this?

[Wladimir Palant, 2015-02-26 22:15:34]

LGTM

I'll need to take a proper look at the results of the conversion script but at
least these changes look good now.

http://codereview.adblockplus.org/5636796054503424/diff/5164320753188864/incl...
File includes/preftable.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5164320753188864/incl...
includes/preftable.tmpl:40: {% elif preference.default is not string %}
On 2015/02/26 22:12:17, kzar wrote:
> I did this because the conversion script annoyingly converts None to null. How
> about this?

I guess we can still get rid of null after the conversion is complete...

[Sebastian Noack, 2015-02-27 11:10:48]

http://codereview.adblockplus.org/5636796054503424/diff/5719887657828352/filt...
File filters/get_subscriptions.py (right):

http://codereview.adblockplus.org/5636796054503424/diff/5719887657828352/filt...
filters/get_subscriptions.py:22: def get_subscriptions(_):
I feel that we begin to misuse template filters. Do you know that in addition to
filters you can also register global functions with Jinja2:
http://jinja.pocoo.org/docs/dev/api/#jinja2.Environment.globals

However, our CMS currently only supports custom filters. But I wonder whether we
should add support for custom globals, for cases like this?

http://codereview.adblockplus.org/5636796054503424/diff/5719887657828352/filt...
filters/get_subscriptions.py:24: source =
urllib.urlopen("https://hg.adblockplus.org/subscriptionlist/archive/default.tar.gz")
The file-like object returned by urllib.urlopen() wants to be closed. ;)

http://codereview.adblockplus.org/5636796054503424/diff/5719887657828352/filt...
filters/get_subscriptions.py:27: if not fileinfo.name.endswith(".subscription"):
Nit: os.path.splitext(fileinfo.name)[1] == ".subscription"

http://codereview.adblockplus.org/5636796054503424/diff/5719887657828352/filt...
filters/get_subscriptions.py:30: filedata = parse_file(fileinfo.name,
codecs.getreader('utf8')(archive.extractfile(fileinfo)))
Nit: Maybe initializing the utf8 reader above the loop, instead calling it on
every iteration?

http://codereview.adblockplus.org/5636796054503424/diff/5719887657828352/incl...
File includes/preftable.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5719887657828352/incl...
includes/preftable.tmpl:23: <th width="30%">{{"prefnamecol"|translate}}</th>
Nit: For consistency, one space before the opening and closing expression.

http://codereview.adblockplus.org/5636796054503424/diff/5719887657828352/incl...
includes/preftable.tmpl:40: {% elif preference.default in (None, null) %}
Just want to make sure that this code is doing what you think it does. It checks
whether "preference.default" is None (built-in) or equals to the value of the
variable "null". But where is this variable defined?

http://codereview.adblockplus.org/5636796054503424/diff/5719887657828352/incl...
File includes/subscriptionList.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5719887657828352/incl...
includes/subscriptionList.tmpl:57:
&amp;requiresLocation={{main_url|urlencode}}&amp;requiresTitle={{main_title|urlencode}}
Nit: For consistency, one space before the opening and closing expression.

[kzar, 2015-02-27 14:21:07]

Addressed snoack's comments.

http://codereview.adblockplus.org/5636796054503424/diff/5719887657828352/filt...
File filters/get_subscriptions.py (right):

http://codereview.adblockplus.org/5636796054503424/diff/5719887657828352/filt...
filters/get_subscriptions.py:24: source =
urllib.urlopen("https://hg.adblockplus.org/subscriptionlist/archive/default.tar.gz")
On 2015/02/27 11:10:49, Sebastian Noack wrote:
> The file-like object returned by urllib.urlopen() wants to be closed. ;)

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5719887657828352/filt...
filters/get_subscriptions.py:27: if not fileinfo.name.endswith(".subscription"):
On 2015/02/27 11:10:49, Sebastian Noack wrote:
> Nit: os.path.splitext(fileinfo.name)[1] == ".subscription"

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5719887657828352/filt...
filters/get_subscriptions.py:30: filedata = parse_file(fileinfo.name,
codecs.getreader('utf8')(archive.extractfile(fileinfo)))
On 2015/02/27 11:10:49, Sebastian Noack wrote:
> Nit: Maybe initializing the utf8 reader above the loop, instead calling it on
> every iteration?

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5719887657828352/incl...
File includes/preftable.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5719887657828352/incl...
includes/preftable.tmpl:23: <th width="30%">{{"prefnamecol"|translate}}</th>
On 2015/02/27 11:10:49, Sebastian Noack wrote:
> Nit: For consistency, one space before the opening and closing expression.

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5719887657828352/incl...
includes/preftable.tmpl:40: {% elif preference.default in (None, null) %}
On 2015/02/27 11:10:49, Sebastian Noack wrote:
> Just want to make sure that this code is doing what you think it does. It
checks
> whether "preference.default" is None (built-in) or equals to the value of the
> variable "null". But where is this variable defined?

I guess `null` is defined by Jinja2, I also noticed this yesterday. This does
work, wherever null is defined, and is required for now. I've tested to make
sure.

http://codereview.adblockplus.org/5636796054503424/diff/5719887657828352/incl...
File includes/subscriptionList.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5719887657828352/incl...
includes/subscriptionList.tmpl:57:
&amp;requiresLocation={{main_url|urlencode}}&amp;requiresTitle={{main_title|urlencode}}
On 2015/02/27 11:10:49, Sebastian Noack wrote:
> Nit: For consistency, one space before the opening and closing expression.

Done.

[Sebastian Noack, 2015-02-27 14:42:45]

http://codereview.adblockplus.org/5636796054503424/diff/5719887657828352/filt...
File filters/get_subscriptions.py (right):

http://codereview.adblockplus.org/5636796054503424/diff/5719887657828352/filt...
filters/get_subscriptions.py:22: def get_subscriptions(_):
On 2015/02/27 11:10:49, Sebastian Noack wrote:
> I feel that we begin to misuse template filters. Do you know that in addition
to
> filters you can also register global functions with Jinja2:
> http://jinja.pocoo.org/docs/dev/api/#jinja2.Environment.globals
> 
> However, our CMS currently only supports custom filters. But I wonder whether
we
> should add support for custom globals, for cases like this?

Since Dave apparently doesn't have an opinion here, what do you think Wladimir?

http://codereview.adblockplus.org/5636796054503424/diff/5719887657828352/incl...
File includes/preftable.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5719887657828352/incl...
includes/preftable.tmpl:40: {% elif preference.default in (None, null) %}
On 2015/02/27 14:21:07, kzar wrote:
> On 2015/02/27 11:10:49, Sebastian Noack wrote:
> > Just want to make sure that this code is doing what you think it does. It
> checks
> > whether "preference.default" is None (built-in) or equals to the value of
the
> > variable "null". But where is this variable defined?
> 
> I guess `null` is defined by Jinja2, I also noticed this yesterday. This does
> work, wherever null is defined, and is required for now. I've tested to make
> sure.

Jinja2 falls back to it's special undefined value when using a non-existing
variable. In this case the above code would be equivalent to:

{% elif preference.default is none or
        preference.default is undefined %}

And this is how you should do it, if that is what you intend to check for.

http://codereview.adblockplus.org/5636796054503424/diff/5668906396024832/filt...
File filters/get_subscriptions.py (right):

http://codereview.adblockplus.org/5636796054503424/diff/5668906396024832/filt...
filters/get_subscriptions.py:37: source.close()
Either:

  source = ...
  try:
    ...
  finally:
    source.close()

Or even better (if supported here):

  with ... as source:
    ...

[kzar, 2015-02-27 14:57:10]

Addressed more of snoack's comments.

http://codereview.adblockplus.org/5636796054503424/diff/5719887657828352/incl...
File includes/preftable.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5719887657828352/incl...
includes/preftable.tmpl:40: {% elif preference.default in (None, null) %}
On 2015/02/27 14:42:45, Sebastian Noack wrote:
> On 2015/02/27 14:21:07, kzar wrote:
> > On 2015/02/27 11:10:49, Sebastian Noack wrote:
> > > Just want to make sure that this code is doing what you think it does. It
> > checks
> > > whether "preference.default" is None (built-in) or equals to the value of
> the
> > > variable "null". But where is this variable defined?
> > 
> > I guess `null` is defined by Jinja2, I also noticed this yesterday. This
does
> > work, wherever null is defined, and is required for now. I've tested to make
> > sure.
> 
> Jinja2 falls back to it's special undefined value when using a non-existing
> variable. In this case the above code would be equivalent to:
> 
> {% elif preference.default is none or
>         preference.default is undefined %}
> 
> And this is how you should do it, if that is what you intend to check for.

Done.

http://codereview.adblockplus.org/5636796054503424/diff/5668906396024832/filt...
File filters/get_subscriptions.py (right):

http://codereview.adblockplus.org/5636796054503424/diff/5668906396024832/filt...
filters/get_subscriptions.py:37: source.close()
On 2015/02/27 14:42:45, Sebastian Noack wrote:
> Either:
> 
>   source = ...
>   try:
>     ...
>   finally:
>     source.close()
> 
> Or even better (if supported here):
> 
>   with ... as source:
>     ...

Unfortunately "with ... as" wasn't supported, I did try that.
http://bugs.python.org/issue12955

Done.

[Sebastian Noack, 2015-02-27 15:03:42]

LGTM. But still, using a filter when you actually need a function isn't great.

[Wladimir Palant, 2015-02-27 15:37:01]

I don't care enough about the splitext() issue mentioned above so LGTM
regardless. I might however revert that change later.

http://codereview.adblockplus.org/5636796054503424/diff/5719887657828352/filt...
File filters/get_subscriptions.py (right):

http://codereview.adblockplus.org/5636796054503424/diff/5719887657828352/filt...
filters/get_subscriptions.py:22: def get_subscriptions(_):
Sure, we are misusing the filters mechanism here, I am fully aware of that. And
we probably want to support custom global functions in the long term. However,
I'd rather not touch the CMS right now and finish the migration sooner - this
isn't the only hack we left around, and it's a comparably small one. I'm all for
filing a follow-up issue on this however.

http://codereview.adblockplus.org/5636796054503424/diff/5750345418407936/filt...
File filters/get_subscriptions.py (right):

http://codereview.adblockplus.org/5636796054503424/diff/5750345418407936/filt...
filters/get_subscriptions.py:30: if os.path.splitext(fileinfo.name)[1] !=
".subscription":
Actually, I'm unsure that this is really better than using endswith() - we don't
have a file system path here, rather a path within a tar archive which might
have different conventions (Unix-like paths regardless of operating system).

[Sebastian Noack, 2015-02-27 15:40:43]

http://codereview.adblockplus.org/5636796054503424/diff/5719887657828352/filt...
File filters/get_subscriptions.py (right):

http://codereview.adblockplus.org/5636796054503424/diff/5719887657828352/filt...
filters/get_subscriptions.py:22: def get_subscriptions(_):
On 2015/02/27 15:37:02, Wladimir Palant wrote:
> Sure, we are misusing the filters mechanism here, I am fully aware of that.
And
> we probably want to support custom global functions in the long term. However,
> I'd rather not touch the CMS right now and finish the migration sooner - this
> isn't the only hack we left around, and it's a comparably small one. I'm all
for
> filing a follow-up issue on this however.

Sounds good to me.

http://codereview.adblockplus.org/5636796054503424/diff/5750345418407936/filt...
File filters/get_subscriptions.py (right):

http://codereview.adblockplus.org/5636796054503424/diff/5750345418407936/filt...
filters/get_subscriptions.py:30: if os.path.splitext(fileinfo.name)[1] !=
".subscription":
On 2015/02/27 15:37:02, Wladimir Palant wrote:
> Actually, I'm unsure that this is really better than using endswith() - we
don't
> have a file system path here, rather a path within a tar archive which might
> have different conventions (Unix-like paths regardless of operating system).

AFAIK splitext() is implemented identically for all file systems. But if in
doubt you could still use posixpath.split().

[Sebastian Noack, 2015-02-27 15:41:18]

http://codereview.adblockplus.org/5636796054503424/diff/5750345418407936/filt...
File filters/get_subscriptions.py (right):

http://codereview.adblockplus.org/5636796054503424/diff/5750345418407936/filt...
filters/get_subscriptions.py:30: if os.path.splitext(fileinfo.name)[1] !=
".subscription":
On 2015/02/27 15:40:43, Sebastian Noack wrote:
> On 2015/02/27 15:37:02, Wladimir Palant wrote:
> > Actually, I'm unsure that this is really better than using endswith() - we
> don't
> > have a file system path here, rather a path within a tar archive which might
> > have different conventions (Unix-like paths regardless of operating system).
> 
> AFAIK splitext() is implemented identically for all file systems. But if in
> doubt you could still use posixpath.split().

Operating systems, I meant.

[Wladimir Palant, 2015-02-27 16:19:57]

http://codereview.adblockplus.org/5636796054503424/diff/5750345418407936/filt...
File filters/get_subscriptions.py (right):

http://codereview.adblockplus.org/5636796054503424/diff/5750345418407936/filt...
filters/get_subscriptions.py:30: if os.path.splitext(fileinfo.name)[1] !=
".subscription":
On 2015/02/27 15:40:43, Sebastian Noack wrote:
> AFAIK splitext() is implemented identically for all file systems.

Sure, it is. However, this doesn't change the fact that here we are applying it
to something it isn't meant for - this isn't a file system path.

[Sebastian Noack, 2015-02-27 16:32:32]

http://codereview.adblockplus.org/5636796054503424/diff/5750345418407936/filt...
File filters/get_subscriptions.py (right):

http://codereview.adblockplus.org/5636796054503424/diff/5750345418407936/filt...
filters/get_subscriptions.py:30: if os.path.splitext(fileinfo.name)[1] !=
".subscription":
On 2015/02/27 16:19:58, Wladimir Palant wrote:
> On 2015/02/27 15:40:43, Sebastian Noack wrote:
> > AFAIK splitext() is implemented identically for all file systems.
> 
> Sure, it is. However, this doesn't change the fact that here we are applying
it
> to something it isn't meant for - this isn't a file system path.

As I said, strictly it would be more correct to use posixpath.splitext() here.
But IMO since splitext() is the same for all operating systems, one could argue
that importing posixpath just for this purpose isn't worth it. Let's leave it up
to Dave, what he prefers. I don't mind.

[kzar, 2015-02-27 16:40:11]

On 2015/02/27 16:32:32, Sebastian Noack wrote:
>
http://codereview.adblockplus.org/5636796054503424/diff/5750345418407936/filt...
> File filters/get_subscriptions.py (right):
> 
>
http://codereview.adblockplus.org/5636796054503424/diff/5750345418407936/filt...
> filters/get_subscriptions.py:30: if os.path.splitext(fileinfo.name)[1] !=
> ".subscription":
> On 2015/02/27 16:19:58, Wladimir Palant wrote:
> > On 2015/02/27 15:40:43, Sebastian Noack wrote:
> > > AFAIK splitext() is implemented identically for all file systems.
> > 
> > Sure, it is. However, this doesn't change the fact that here we are applying
> it
> > to something it isn't meant for - this isn't a file system path.
> 
> As I said, strictly it would be more correct to use posixpath.splitext() here.
> But IMO since splitext() is the same for all operating systems, one could
argue
> that importing posixpath just for this purpose isn't worth it. Let's leave it
up
> to Dave, what he prefers. I don't mind.

My opinion is it works, let's leave it alone and hopefully get the changes
pushed. We need to get away from Anwiki, we can tweak stuff like this after the
migration is finished if we really need to.

[Wladimir Palant, 2015-03-06 23:21:06]

As explained below, I made some incompatible changes to the converter script.
I'll send you my modifications to the templates here.

http://codereview.adblockplus.org/5636796054503424/diff/5750345418407936/filt...
File filters/parse_interface.py (right):

http://codereview.adblockplus.org/5636796054503424/diff/5750345418407936/filt...
filters/parse_interface.py:49: })
With my changes value has a lot more properties and these properties need to be
kept. My suggestion would be to add properties to value rather than creating a
new dictionary. So value.update({...}) here and then parsed.append(value). Same
for properties below.

http://codereview.adblockplus.org/5636796054503424/diff/5750345418407936/incl...
File includes/interface.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5750345418407936/incl...
includes/interface.tmpl:17: {% from "includes/common" import description with
context %}
I changed the converter script and implemented a better solution here. Instead
of implicit assumptions about the structure of various descriptions,
corresponding localization strings and mapping to links the template defines a
bunch of macros for the descriptions. It then references these macros as part of
the interface structure - "description" property is a macro that generates
property/method description, there are also "description-arg" and
"description-return" properties. The macros are localized exactly like all the
pages we have.

So in order to add the description for a property this template now only needs
to say:

{{ property.description() }}

For a method argument it is slightly more complicated:

{{ property["description-" + argument.name]() }}

It would be great if you could implement these changes.

http://codereview.adblockplus.org/5636796054503424/diff/5750345418407936/incl...
File includes/preftable.tmpl (right):

http://codereview.adblockplus.org/5636796054503424/diff/5750345418407936/incl...
includes/preftable.tmpl:17: {% from "includes/common" import description with
context %}
I changed the converter script here as well, this template will now get macros
as part of preference definition. So preference description is generated by
simply calling the macro:

{{ preference.description() }}

I also made sure that the section titles are set explicitly as part of the data
structure (again, no implicit assumptions about the string names). So {{
(section.id + "Title")|translate }} should become {{ section.title }}.

[kzar, 2015-03-07 18:39:24]

Patch Set 13 : Wladimir's interface + preference file changes for compatibility
with conversion script.

(I fixed line endings for one file that was set to DOS mode and therefore
messing up the diff, also a Maxthon image snuck in!)

I didn't write these changes, although I have re-tested the site and they seem
to work fine. I've not yet addressed the extra comments but I figured they
should be in a separate patch set.

[kzar, 2015-03-07 19:12:59]

Patch Set 14 : Remove includes/common.tmpl which is no longer required.

[kzar, 2015-03-07 19:16:58]

Your changes LGTM Wladimir.

I've mostly left everything alone, just deleted that descriptions macro we no
longer need, and tweaked the conversion script to ensure safari.raw is deleted
as it keeps being re-created and breaking the Safari landing page. IMHO this is
all pretty much ready to be pushed to wwwnew now.

[Wladimir Palant, 2015-03-07 19:21:57]

Yes, LGTM. There are a few relatively minor issues left but these affect the
conversion script. I think we can update the repo and improve the conversion
script later.