Issue 3354 - Enable extras module for Markdown inside HTML blocks

Issue 3354 - Enable extras module for Markdown inside HTML blocks

[kzar, 2015-11-26 13:31:15]

Patch Set 1

[saroyanm, 2015-11-26 13:49:50]

https://codereview.adblockplus.org/29331049/diff/29331050/cms/converters.py
File cms/converters.py (right):

https://codereview.adblockplus.org/29331049/diff/29331050/cms/converters.py#n...
cms/converters.py:314: extensions=["attr_list", "markdown.extensions.extra"])
According to documentation extra extension include attr_list extension.
https://pythonhosted.org/Markdown/extensions/extra.html 
Also make sense to update README.md, replacing "Attribute Lists" reference to
the "extra" extension.

[kzar, 2015-11-26 14:01:34]

Patch Set 2 : Removed redundant attr_list extension, updated README

https://codereview.adblockplus.org/29331049/diff/29331050/cms/converters.py
File cms/converters.py (right):

https://codereview.adblockplus.org/29331049/diff/29331050/cms/converters.py#n...
cms/converters.py:314: extensions=["attr_list", "markdown.extensions.extra"])
On 2015/11/26 13:49:50, saroyanm wrote:
> According to documentation extra extension include attr_list extension.
> https://pythonhosted.org/Markdown/extensions/extra.html 
> Also make sense to update README.md, replacing "Attribute Lists" reference to
> the "extra" extension.

I read the documentation differently, but I gave it a try and it turns out
you're right. Done.

[Sebastian Noack, 2015-11-26 14:23:51]

https://codereview.adblockplus.org/29331049/diff/29331050/cms/converters.py
File cms/converters.py (right):

https://codereview.adblockplus.org/29331049/diff/29331050/cms/converters.py#n...
cms/converters.py:314: extensions=["attr_list", "markdown.extensions.extra"])
On 2015/11/26 14:01:34, kzar wrote:
> On 2015/11/26 13:49:50, saroyanm wrote:
> > According to documentation extra extension include attr_list extension.
> > https://pythonhosted.org/Markdown/extensions/extra.html 
> > Also make sense to update README.md, replacing "Attribute Lists" reference
to
> > the "extra" extension.
> 
> I read the documentation differently, but I gave it a try and it turns out
> you're right. Done.

Well, this is weird, but seems to be right. While there is a separate extension
for other features included with the "extra" extension. HTML block support is
only available if you enable the whole "extra" extension collection.

https://codereview.adblockplus.org/29331049/diff/29331052/README.md
File README.md (right):

https://codereview.adblockplus.org/29331049/diff/29331052/README.md#newcode211
README.md:211: HTML tags, and HTML block elements that contain Markdown.
I think there isn't supposed to be a comma before the "and" in an enumeration.

https://codereview.adblockplus.org/29331049/diff/29331052/README.md#newcode211
README.md:211: HTML tags, and HTML block elements that contain Markdown.
Apparently, that is not the only thing the extra extension allows. Listing all
of them isn't probably necessary though. Just add " among others", " and other
features" or something like that.

https://codereview.adblockplus.org/29331049/diff/29331052/cms/converters.py
File cms/converters.py (right):

https://codereview.adblockplus.org/29331049/diff/29331052/cms/converters.py#n...
cms/converters.py:314: extensions=["markdown.extensions.extra"])
Do we actually have to use the long form for "extra" when we didn't for
"attr_list"?

[saroyanm, 2015-11-26 14:29:48]

https://codereview.adblockplus.org/29331049/diff/29331050/cms/converters.py
File cms/converters.py (right):

https://codereview.adblockplus.org/29331049/diff/29331050/cms/converters.py#n...
cms/converters.py:314: extensions=["attr_list", "markdown.extensions.extra"])
On 2015/11/26 14:01:34, kzar wrote:
> On 2015/11/26 13:49:50, saroyanm wrote:
> > According to documentation extra extension include attr_list extension.
> > https://pythonhosted.org/Markdown/extensions/extra.html 
> > Also make sense to update README.md, replacing "Attribute Lists" reference
to
> > the "extra" extension.
> 
> I read the documentation differently, but I gave it a try and it turns out
> you're right. Done.

Yes I also had a doubt initially, they formulated it not clear, but it's
imitation of the PHP Extra ( https://michelf.ca/projects/php-markdown/extra/ ),
which do include the attribute list implementation, also I've checked the link
below according to which looks my doubts went away:
https://pythonhosted.org/Markdown/extensions/index.html#officially-supported-...

https://codereview.adblockplus.org/29331049/diff/29331052/README.md
File README.md (right):

https://codereview.adblockplus.org/29331049/diff/29331052/README.md#newcode211
README.md:211: HTML tags, and HTML block elements that contain Markdown.
Maybe make sense to make description more general, while it's not only 2
features that "extra" extenstion provide, smth like:
[Python-Markdown Extra](https://pythonhosted.org/Markdown/extensions/extra.html)
extension is active and provide imitation of [php Markdown
Extra](https://michelf.ca/projects/php-markdown/extra/) extension.

Feel free to list all included extensions, but IMO for users this information
with reference should be enough.

[kzar, 2015-11-26 14:41:24]

Patch Set 3 : Addressed Sebastian's feedback

https://codereview.adblockplus.org/29331049/diff/29331052/README.md
File README.md (right):

https://codereview.adblockplus.org/29331049/diff/29331052/README.md#newcode211
README.md:211: HTML tags, and HTML block elements that contain Markdown.
On 2015/11/26 14:23:51, Sebastian Noack wrote:
> Apparently, that is not the only thing the extra extension allows. Listing all
> of them isn't probably necessary though. Just add " among others", " and other
> features" or something like that.

Done.

https://codereview.adblockplus.org/29331049/diff/29331052/cms/converters.py
File cms/converters.py (right):

https://codereview.adblockplus.org/29331049/diff/29331052/cms/converters.py#n...
cms/converters.py:314: extensions=["markdown.extensions.extra"])
On 2015/11/26 14:23:51, Sebastian Noack wrote:
> Do we actually have to use the long form for "extra" when we didn't for
> "attr_list"?

No, just tested it and "extra" works fine. Done.

[saroyanm, 2015-11-26 14:44:35]

LGTM

[Sebastian Noack, 2015-11-26 21:17:46]

You forgot to adapt the hidden imports in runserver.spec

[kzar, 2015-11-27 09:11:17]

Patch Set 4 : Update hidden dependency in runserver.spec

https://codereview.adblockplus.org/29331049/diff/29331101/runserver.spec
File runserver.spec (right):

https://codereview.adblockplus.org/29331049/diff/29331101/runserver.spec#newc...
runserver.spec:22: 'markdown.extensions.extra',
(I was not able to test this change as I could not get pyinstaller to work on my
system.)

[Sebastian Noack, 2015-11-27 09:28:34]

https://codereview.adblockplus.org/29331049/diff/29331101/runserver.spec
File runserver.spec (right):

https://codereview.adblockplus.org/29331049/diff/29331101/runserver.spec#newc...
runserver.spec:22: 'markdown.extensions.extra',
That isn't sufficient as the extensions included with extra are lazy loaded as
well:
https://github.com/waylan/Python-Markdown/blob/master/markdown/extensions/ext...

[kzar, 2015-11-27 09:45:31]

Patch Set 5 : Add all Markdown extra extensions to hiddenimports list

https://codereview.adblockplus.org/29331049/diff/29331101/runserver.spec
File runserver.spec (right):

https://codereview.adblockplus.org/29331049/diff/29331101/runserver.spec#newc...
runserver.spec:22: 'markdown.extensions.extra',
On 2015/11/27 09:28:33, Sebastian Noack wrote:
> That isn't sufficient as the extensions included with extra are lazy loaded as
> well:
>
https://github.com/waylan/Python-Markdown/blob/master/markdown/extensions/ext...

Done.

[Sebastian Noack, 2015-11-27 09:47:45]

LGTM