Issue 3402 - Properly handle nested inline tags

Issue 3402 - Properly handle nested inline tags

[kzar, 2015-12-22 13:52:34]

Patch Set 1

[Sebastian Noack, 2015-12-22 15:14:18]

I put Wladimir as reviewer since I'm on vacation.

[Sebastian Noack, 2016-01-21 13:55:56]

https://codereview.adblockplus.org/29332956/diff/29332957/cms/converters.py
File cms/converters.py (right):

https://codereview.adblockplus.org/29332956/diff/29332957/cms/converters.py#n...
cms/converters.py:185: for t in ("\<%s[^<>]*?\>", "\<\/%s\>")
Nit: None of your escaping applies to regexp as you don't use double backslashes
or  r"..". However, that shows that escaping isn't even necessary here.

Nit: You could simplify the surrounding Python code if you use a single regexp:
"<(?:%s[^<>]*?|/%s)>"

https://codereview.adblockplus.org/29332956/diff/29332957/cms/converters.py#n...
cms/converters.py:186: for tag_name in self.whitelist.difference({tag})]
Nit: self.whitelist - {tag}

Besides being more concise, it's probably also easier to understand as it's
clear that arithmetic semantics apply and you don't wonder whether
Set.difference() is some symmetric operation.

https://codereview.adblockplus.org/29332956/diff/29332957/cms/converters.py#n...
cms/converters.py:192: r"%s(%s*?)%s" % (re_escape("<%s>" % tag),
allowed_contents,
I wonder whether you should parse the content recursively to make sure that tags
are closed correctly.

[kzar, 2016-02-19 14:32:29]

Patch Set 2 : Addressed feedback

https://codereview.adblockplus.org/29332956/diff/29332957/cms/converters.py
File cms/converters.py (right):

https://codereview.adblockplus.org/29332956/diff/29332957/cms/converters.py#n...
cms/converters.py:185: for t in ("\<%s[^<>]*?\>", "\<\/%s\>")
On 2016/01/21 13:55:56, Sebastian Noack wrote:
> Nit: None of your escaping applies to regexp as you don't use double
backslashes
> or  r"..". However, that shows that escaping isn't even necessary here.
> 
> Nit: You could simplify the surrounding Python code if you use a single
regexp:
> "<(?:%s[^<>]*?|/%s)>"

Done.

https://codereview.adblockplus.org/29332956/diff/29332957/cms/converters.py#n...
cms/converters.py:186: for tag_name in self.whitelist.difference({tag})]
On 2016/01/21 13:55:56, Sebastian Noack wrote:
> Nit: self.whitelist - {tag}
> 
> Besides being more concise, it's probably also easier to understand as it's
> clear that arithmetic semantics apply and you don't wonder whether
> Set.difference() is some symmetric operation.

Done.

https://codereview.adblockplus.org/29332956/diff/29332957/cms/converters.py#n...
cms/converters.py:192: r"%s(%s*?)%s" % (re_escape("<%s>" % tag),
allowed_contents,
On 2016/01/21 13:55:56, Sebastian Noack wrote:
> I wonder whether you should parse the content recursively to make sure that
tags
> are closed correctly.

I guess so but probably overkill, we didn't check that before.

[Sebastian Noack, 2016-03-20 15:14:11]

LGTM