Noissue - Refactor Converter.__init__ and __call__

Noissue - Refactor Converter.__init__ and __call__

Repository: https://hg.adblockplus.org/cms/
Base revision: 7348838c1e8f

[Vasily Kuznetsov, 2017-10-27 18:35:02]

Hi Matze,

I'm planning to implement a few changes to make the code of the CMS easier to
follow. This one addresses issues with the `Converter` class: the side-effects
of its constructor and variable return type of `__call__` method.

Let me know what you think.

Cheers,
Vasily

P.S. I tested this change by generating web.acceptableads.com,
web.acceptableads.org, web.adblockbrowser.org, web.adblockplus.org and
web.eyeo.com with new and old version of CMS.

https://codereview.adblockplus.org/29590620/diff/29590621/cms/converters.py
File cms/converters.py (left):

https://codereview.adblockplus.org/29590620/diff/29590621/cms/converters.py#o...
cms/converters.py:121: def parse_page_content(page, data):
This function has nothing to do with converters, it logic related to reading the
source of a page. Pages don't have a dedicated source file yet, so for now it
goes to utils.

https://codereview.adblockplus.org/29590620/diff/29590621/cms/converters.py#o...
cms/converters.py:145: def __init__(self, params, key='pagedata'):
This signature is rather cryptic, what we want to say here is that the converter
should render params[key]. I made it more straight forward.

https://codereview.adblockplus.org/29590620/diff/29590621/cms/converters.py#o...
cms/converters.py:153: data, filename = params[key]
This is a side effect that is not related to converting from source formats to
HTML (the job of Converter class). It should not be in the Converter
constructor.

https://codereview.adblockplus.org/29590620/diff/29590621/cms/converters.py#o...
cms/converters.py:298: self._params['includedata'] = (
This key is not used anywhere, so we don't need to write this into `params`.

https://codereview.adblockplus.org/29590620/diff/29590621/cms/converters.py#o...
cms/converters.py:322: if self._key == 'pagedata':
The return types of the two branches of if are different. This is confusing and
brittle. The code for decapitating the pages is moved to utils and is used there
explicitly.

https://codereview.adblockplus.org/29590620/diff/29590621/cms/converters.py
File cms/converters.py (right):

https://codereview.adblockplus.org/29590620/diff/29590621/cms/converters.py#n...
cms/converters.py:26: from cms import utils
It makes more sense for `utils` to be imported into `converters` than the other
way around. Right now it just moved the worry about the import loop into
`utils`, but the code that imports converters there should be in a more proper
module anyway, so eventually the import loop will disappear and we'll be able to
import `utils` here. For now let's pretend that we already can ;)

https://codereview.adblockplus.org/29590620/diff/29590621/cms/converters.py#n...
cms/converters.py:280: metadata, rest = utils.extract_page_metadata(data)
We have to maintain the ability of includes to write into `params` of the
including page for compatibility reasons. Hopefully not forever, because this
seems like bad practice in the website construction. At least this is now
explicit here

https://codereview.adblockplus.org/29590620/diff/29590621/cms/utils.py
File cms/utils.py (right):

https://codereview.adblockplus.org/29590620/diff/29590621/cms/utils.py#newcode26
cms/utils.py:26: def split_head_body(html):
Maybe this function should be called `decapitate`...

https://codereview.adblockplus.org/29590620/diff/29590621/cms/utils.py#newcode87
cms/utils.py:87: from cms.converters import converters
It makes more sense to do contrived imports on this side of the import loop (not
in `converters`) because `utils` should in theory be a small and light module
with no dependencies (and I hope it will eventually become that).

https://codereview.adblockplus.org/29590620/diff/29590621/cms/utils.py#newcod...
cms/utils.py:117: data, filename = source.read_page(page, format)
This was done in the constructor of `Converter` before, as a side effect.

https://codereview.adblockplus.org/29590620/diff/29590621/cms/utils.py#newcod...
cms/utils.py:137: try:
Lines 137-143 should probably be in `converters.py` since it's basically
converter-related logic. For now let them be here.

https://codereview.adblockplus.org/29590620/diff/29590621/tests/test_site/inc...
File tests/test_site/includes/example.html (right):

https://codereview.adblockplus.org/29590620/diff/29590621/tests/test_site/inc...
tests/test_site/includes/example.html:1: foo = bar
We need this included fragment to test that includes still work and that they
can still write into `params` of the parent page.

https://codereview.adblockplus.org/29590620/diff/29590621/tests/test_site/tem...
File tests/test_site/templates/include-test.tmpl (right):

https://codereview.adblockplus.org/29590620/diff/29590621/tests/test_site/tem...
tests/test_site/templates/include-test.tmpl:2: Variable from included piece: {{
foo }}
Check that the include was able to write into `params` of the parent page.

[mathias, 2017-10-30 15:37:01]

https://codereview.adblockplus.org/29590620/diff/29590621/cms/converters.py
File cms/converters.py (right):

https://codereview.adblockplus.org/29590620/diff/29590621/cms/converters.py#n...
cms/converters.py:26: from cms import utils
On 2017/10/27 18:35:01, Vasily Kuznetsov wrote:
> It makes more sense for `utils` to be imported into `converters` than the
other
> way around. Right now it just moved the worry about the import loop into
> `utils`, but the code that imports converters there should be in a more proper
> module anyway, so eventually the import loop will disappear and we'll be able
to
> import `utils` here. For now let's pretend that we already can ;)

Acknowledged.

https://codereview.adblockplus.org/29590620/diff/29590621/cms/converters.py#n...
cms/converters.py:280: metadata, rest = utils.extract_page_metadata(data)
On 2017/10/27 18:35:01, Vasily Kuznetsov wrote:
> We have to maintain the ability of includes to write into `params` of the
> including page for compatibility reasons. Hopefully not forever, because this
> seems like bad practice in the website construction. At least this is now
> explicit here

Acknowledged.

https://codereview.adblockplus.org/29590620/diff/29590621/cms/converters.py#n...
cms/converters.py:471: page_data['page'] = page_name
Shouldn't this use setdefault(), in order to allow for the metadata to include a
value itself (for whatever reason)?

https://codereview.adblockplus.org/29590620/diff/29590621/cms/utils.py
File cms/utils.py (right):

https://codereview.adblockplus.org/29590620/diff/29590621/cms/utils.py#newcode26
cms/utils.py:26: def split_head_body(html):
On 2017/10/27 18:35:01, Vasily Kuznetsov wrote:
> Maybe this function should be called `decapitate`...

Funny, but no. The name is perfect, there's nothing much to guess.

https://codereview.adblockplus.org/29590620/diff/29590621/cms/utils.py#newcode87
cms/utils.py:87: from cms.converters import converters
On 2017/10/27 18:35:02, Vasily Kuznetsov wrote:
> It makes more sense to do contrived imports on this side of the import loop
(not
> in `converters`) because `utils` should in theory be a small and light module
> with no dependencies (and I hope it will eventually become that).

Acknowledged.

https://codereview.adblockplus.org/29590620/diff/29590621/cms/utils.py#newcod...
cms/utils.py:117: data, filename = source.read_page(page, format)
On 2017/10/27 18:35:01, Vasily Kuznetsov wrote:
> This was done in the constructor of `Converter` before, as a side effect.

Acknowledged.

https://codereview.adblockplus.org/29590620/diff/29590621/cms/utils.py#newcod...
cms/utils.py:137: try:
On 2017/10/27 18:35:02, Vasily Kuznetsov wrote:
> Lines 137-143 should probably be in `converters.py` since it's basically
> converter-related logic. For now let them be here.

Acknowledged.

https://codereview.adblockplus.org/29590620/diff/29590621/cms/utils.py#newcod...
cms/utils.py:139: converter = converter_class(body, filename, params)
This line should not be part of this try-block any more.

https://codereview.adblockplus.org/29590620/diff/29590621/tests/test_site/inc...
File tests/test_site/includes/example.html (right):

https://codereview.adblockplus.org/29590620/diff/29590621/tests/test_site/inc...
tests/test_site/includes/example.html:1: foo = bar
On 2017/10/27 18:35:02, Vasily Kuznetsov wrote:
> We need this included fragment to test that includes still work and that they
> can still write into `params` of the parent page.

Acknowledged.

https://codereview.adblockplus.org/29590620/diff/29590621/tests/test_site/tem...
File tests/test_site/templates/include-test.tmpl (right):

https://codereview.adblockplus.org/29590620/diff/29590621/tests/test_site/tem...
tests/test_site/templates/include-test.tmpl:2: Variable from included piece: {{
foo }}
On 2017/10/27 18:35:02, Vasily Kuznetsov wrote:
> Check that the include was able to write into `params` of the parent page.

Acknowledged.

[Vasily Kuznetsov, 2017-11-07 17:08:30]

Hi Matze,

Thanks for your comments, I've addressed them.

I'm wondering if we should add a test for a page overriding its own name in the
medadata but I'm not so sure we want to have this feature. We should probably
decide that then and see about the test. For now I can note that the behavior is
not changing and overriding page name is not used anywhere in practice (because
my previous testing with real websites passed).

Let me know if you see anything else wrong with the code.

Cheers,
Vasily

https://codereview.adblockplus.org/29590620/diff/29590621/cms/converters.py
File cms/converters.py (right):

https://codereview.adblockplus.org/29590620/diff/29590621/cms/converters.py#n...
cms/converters.py:471: page_data['page'] = page_name
On 2017/10/30 15:37:00, mathias wrote:
> Shouldn't this use setdefault(), in order to allow for the metadata to include
a
> value itself (for whatever reason)?

Yeah, you're right, this would be needed to preserve existing behavior.
Done.

https://codereview.adblockplus.org/29590620/diff/29590621/cms/utils.py
File cms/utils.py (right):

https://codereview.adblockplus.org/29590620/diff/29590621/cms/utils.py#newcod...
cms/utils.py:139: converter = converter_class(body, filename, params)
On 2017/10/30 15:37:00, mathias wrote:
> This line should not be part of this try-block any more.

Done.

[mathias, 2017-11-07 19:49:48]

LGTM.