Fixes 6546 - Swap the contents of default.tmpl and empty.tmpl in the test suite

Fixes 6546 - Swap the contents of default.tmpl and empty.tmpl in the test suite

Repository: https://hg.adblockplus.org/cms
Base revision: b683e321a9d0

[Vasily Kuznetsov, 2018-04-03 14:25:02]

This change fixes the roadblock for https://codereview.adblockplus.org/29516687/

[Sebastian Noack, 2018-04-03 22:32:51]

LGTM

https://codereview.adblockplus.org/29741581/diff/29741641/tests/expected_outp...
File tests/expected_output/en/sitemap (left):

https://codereview.adblockplus.org/29741581/diff/29741641/tests/expected_outp...
tests/expected_output/en/sitemap:17: <li>title : translate-tmpl </li>
So these pages showed up here before because they defined any metadata (i.e. the
template), but since they didn't define any other metadata they are no longer
considered by get_pages_metadata(). I still fail to see why these semantics make
any sense. But not my business, and somewhat unrelated of this change anyway.

[Vasily Kuznetsov, 2018-04-03 22:47:05]

https://codereview.adblockplus.org/29741581/diff/29741641/tests/expected_outp...
File tests/expected_output/en/sitemap (left):

https://codereview.adblockplus.org/29741581/diff/29741641/tests/expected_outp...
tests/expected_output/en/sitemap:17: <li>title : translate-tmpl </li>
On 2018/04/03 22:32:51, Sebastian Noack wrote:
> So these pages showed up here before because they defined any metadata (i.e.
the
> template), but since they didn't define any other metadata they are no longer
> considered by get_pages_metadata(). I still fail to see why these semantics
make
> any sense. But not my business, and somewhat unrelated of this change anyway.

I pretty much see it the same way as you do, but there's another possible point
of view: `get_pages_metadata` returns user-defined metadata dicts from the pages
that have user-defined metadata. Arguably it's less consistent, but in practice
(and for real use cases of this function) it makes no difference because all the
pages that people would ever want to get out if it would have user-defined
metadata. Julian (who actually uses CMS and this function) prefers second
interpretation and I think this is not important enough to have debates about,
so I'll just let it be.

[rosie, 2018-04-04 16:22:17]

LGTM, just a quick note about making the expected behavior of
`get_pages_metadata` a bit clearer with a comment.

https://codereview.adblockplus.org/29741581/diff/29741641/tests/expected_outp...
File tests/expected_output/en/sitemap (left):

https://codereview.adblockplus.org/29741581/diff/29741641/tests/expected_outp...
tests/expected_output/en/sitemap:17: <li>title : translate-tmpl </li>
On 2018/04/03 22:47:04, Vasily Kuznetsov wrote:
> On 2018/04/03 22:32:51, Sebastian Noack wrote:
> > So these pages showed up here before because they defined any metadata (i.e.
> the
> > template), but since they didn't define any other metadata they are no
longer
> > considered by get_pages_metadata(). I still fail to see why these semantics
> make
> > any sense. But not my business, and somewhat unrelated of this change
anyway.
> 
> I pretty much see it the same way as you do, but there's another possible
point
> of view: `get_pages_metadata` returns user-defined metadata dicts from the
pages
> that have user-defined metadata. Arguably it's less consistent, but in
practice
> (and for real use cases of this function) it makes no difference because all
the
> pages that people would ever want to get out if it would have user-defined
> metadata. Julian (who actually uses CMS and this function) prefers second
> interpretation and I think this is not important enough to have debates about,
> so I'll just let it be.

Should there be a comment somewhere in this file that makes the expected
behavior clearer?

[Vasily Kuznetsov, 2018-04-04 16:29:37]

On 2018/04/04 16:22:17, rosie wrote:
> LGTM, just a quick note about making the expected behavior of
> `get_pages_metadata` a bit clearer with a comment.
> 
>
https://codereview.adblockplus.org/29741581/diff/29741641/tests/expected_outp...
> File tests/expected_output/en/sitemap (left):
> 
>
https://codereview.adblockplus.org/29741581/diff/29741641/tests/expected_outp...
> tests/expected_output/en/sitemap:17: <li>title : translate-tmpl </li>
> On 2018/04/03 22:47:04, Vasily Kuznetsov wrote:
> > On 2018/04/03 22:32:51, Sebastian Noack wrote:
> > > So these pages showed up here before because they defined any metadata
(i.e.
> > the
> > > template), but since they didn't define any other metadata they are no
> longer
> > > considered by get_pages_metadata(). I still fail to see why these
semantics
> > make
> > > any sense. But not my business, and somewhat unrelated of this change
> anyway.
> > 
> > I pretty much see it the same way as you do, but there's another possible
> point
> > of view: `get_pages_metadata` returns user-defined metadata dicts from the
> pages
> > that have user-defined metadata. Arguably it's less consistent, but in
> practice
> > (and for real use cases of this function) it makes no difference because all
> the
> > pages that people would ever want to get out if it would have user-defined
> > metadata. Julian (who actually uses CMS and this function) prefers second
> > interpretation and I think this is not important enough to have debates
about,
> > so I'll just let it be.
> 
> Should there be a comment somewhere in this file that makes the expected
> behavior clearer?

It would be good to document this behavior (probably the docstring of
`get_pages_metadata` is the best place), but this is not related to this change,
so it probably should be a separate noissue review.

[Sebastian Noack, 2018-04-04 17:42:58]

https://codereview.adblockplus.org/29741581/diff/29741641/tests/expected_outp...
File tests/expected_output/en/sitemap (left):

https://codereview.adblockplus.org/29741581/diff/29741641/tests/expected_outp...
tests/expected_output/en/sitemap:17: <li>title : translate-tmpl </li>
On 2018/04/04 16:29:37, Vasily Kuznetsov wrote:
> It would be good to document this behavior (probably the docstring of
> `get_pages_metadata` is the best place), but this is not related to this
change,
> so it probably should be a separate noissue review.

As I pointed out in the other review, there is a comment in the source code:
https://hg.adblockplus.org/cms/file/b683e321a9d0/cms/converters.py#l477
But its merely indicating that pages without custom metadata are skipped, but
fails to explain why.

Furthermore, it seems there is out-of-source documentation for provided template
functions in docs/api/functions.md, which however is missing any mention of
get_pages_metadata() at all.