Noissue - Use selectattr() filter to filter page metadata

Noissue - Use selectattr() filter to filter page metadata

[Sebastian Noack, 2018-04-17 06:23:27]

This is a proof of concept, how help.eyeo.com could look like if we remove
filtering functionality from get_pages_metadata() in favor of jinja's built-in
selectattr() filter. See the discussion at
https://issues.adblockplus.org/ticket/6545#comment:7.

[juliandoucette, 2018-04-17 10:43:07]

CC Ire (She has implemented most of the help centre.)

This is too verbose for my liking. I don't even like the name
`get_pages_metadata` because it's long.

If these Jinja2 features implement features we have implemented in
get_pages_metadata then I'd prefer if we used them *inside* get_pages_metadata
instead of *outside*.

What do you think Ire?

[Sebastian Noack, 2018-04-17 12:18:13]

On 2018/04/17 10:43:07, juliandoucette wrote:
> CC Ire (She has implemented most of the help centre.)

> This is too verbose for my liking.

I admit that the format is more verbose, but essentially we trade in a little
more boilerplate here, for a fair amount of complexity in the CMS we could get
rid of, which IMO is a reasonable trade-off. Furthermore, the selectattr()
filter is way more flexible/powerful than the filter functionality currently
provided by get_pages_metadata().

> I don't even like the name `get_pages_metadata`
> because it's long.

If we change the API anyway, we can aalso change the name of
get_pages_metadata() to whatever you want.

> If these Jinja2 features implement features we have implemented in
> get_pages_metadata then I'd prefer if we used them *inside* get_pages_metadata
> instead of *outside*.

This isn't technically possible, neither is this how jinja is meant to be used.
Filters are only provided to templates (in order to avoid redundant complexity
in your controller code, and give more flexibility to template authors), they
cannot be used directly from Python code.

[Vasily Kuznetsov, 2018-04-17 15:12:53]

On 2018/04/17 12:18:13, Sebastian Noack wrote:
> On 2018/04/17 10:43:07, juliandoucette wrote:
> > CC Ire (She has implemented most of the help centre.)
> 
> > This is too verbose for my liking.
> 
> I admit that the format is more verbose, but essentially we trade in a little
> more boilerplate here, for a fair amount of complexity in the CMS we could get
> rid of, which IMO is a reasonable trade-off. Furthermore, the selectattr()
> filter is way more flexible/powerful than the filter functionality currently
> provided by get_pages_metadata().
> 
> > I don't even like the name `get_pages_metadata`
> > because it's long.
> 
> If we change the API anyway, we can aalso change the name of
> get_pages_metadata() to whatever you want.
> 
> > If these Jinja2 features implement features we have implemented in
> > get_pages_metadata then I'd prefer if we used them *inside*
get_pages_metadata
> > instead of *outside*.
> 
> This isn't technically possible, neither is this how jinja is meant to be
used.
> Filters are only provided to templates (in order to avoid redundant complexity
> in your controller code, and give more flexibility to template authors), they
> cannot be used directly from Python code.

The current implementation of `get_pages_metadata` is a fairly independent and
small piece of code in the CMS. I don't think getting rid of filtering there
(~20 lines of code that doesn't call anything else) would have any material
effect on the complexity of the CMS. If it also makes Julian and other website
developers less happy, I would see it as a step in the wrong direction.

One possible solution for "too much stuff in the CMS" problem (although I don't
think we have this problem to any serious extent) would be to move
`get_pages_metadata` into a global in `website-defaults`. There it could become
more specific for the use cases of our websites and could also be called
something shorter. I'm happy to help with this, if needed.

[Sebastian Noack, 2018-04-17 16:20:09]

On 2018/04/17 15:12:53, Vasily Kuznetsov wrote:
> The current implementation of `get_pages_metadata` is a fairly independent and
> small piece of code in the CMS. I don't think getting rid of filtering there
> (~20 lines of code that doesn't call anything else) would have any material
> effect on the complexity of the CMS. If it also makes Julian and other website
> developers less happy, I would see it as a step in the wrong direction.

Well, 20 lines of code (though probably some more when accounting for tests)
might not be a ridiculous amount of complexity, but compared to the little
boilerplate this patch would add, still not really worth to maintain, if it
wouldn't be for the plain purpose of avoiding this debate.

FWIW, one could also see the changes here as an improvement (as opposed to a
compromise in favor of CMS developers). At least the selectattr() filter is a
well-documented feature of a standard tool (i.e. jinja). So every developer who
knows the jinja template language, immediately understands what this template
code does, or at very least everyone can figure it out quickly, without digging
into the CMS. Not to mention that it is more flexible too (i.e. you can filter
by any attribute, in any kind of sequence in a bunch of different ways).

It's not about get_pages_metadata() being part of the CMS. We need some stable
interface towards the websites, and a template function is straight-forward.
Having an implementation of get_pages_metadata() somewhere else relying on
internal implementation details of the CMS, will just cause an even larger mess.
It is more about not reinventing the wheel (and then insisting on making it
square-shaped). Did you ever complain that you have to use grep and not have
(more limited) output filtering built into every single command instead?

[Jon Sonesen, 2018-04-17 18:01:41]

On 2018/04/17 16:20:09, Sebastian Noack wrote:
> On 2018/04/17 15:12:53, Vasily Kuznetsov wrote:
> > The current implementation of `get_pages_metadata` is a fairly independent
and
> > small piece of code in the CMS. I don't think getting rid of filtering there
> > (~20 lines of code that doesn't call anything else) would have any material
> > effect on the complexity of the CMS. If it also makes Julian and other
website
> > developers less happy, I would see it as a step in the wrong direction.
> 
> Well, 20 lines of code (though probably some more when accounting for tests)
> might not be a ridiculous amount of complexity, but compared to the little
> boilerplate this patch would add, still not really worth to maintain, if it
> wouldn't be for the plain purpose of avoiding this debate.
> 
> FWIW, one could also see the changes here as an improvement (as opposed to a
> compromise in favor of CMS developers). At least the selectattr() filter is a
> well-documented feature of a standard tool (i.e. jinja). So every developer
who
> knows the jinja template language, immediately understands what this template
> code does, or at very least everyone can figure it out quickly, without
digging
> into the CMS. Not to mention that it is more flexible too (i.e. you can filter
> by any attribute, in any kind of sequence in a bunch of different ways).
> 
> It's not about get_pages_metadata() being part of the CMS. We need some stable
> interface towards the websites, and a template function is straight-forward.
> Having an implementation of get_pages_metadata() somewhere else relying on
> internal implementation details of the CMS, will just cause an even larger
mess.
> It is more about not reinventing the wheel (and then insisting on making it
> square-shaped). Did you ever complain that you have to use grep and not have
> (more limited) output filtering built into every single command instead?


I am inclined to agree with vasily here, there are much more problematic areas
of the CMS than this feature and it has not caused any demonstrable confusion
that I have been made aware of. In fact, I think that since our only users are
the website developers we should do what they prefer. This filter feature was
requested specifically to simplify the API and I have been aware of selectattr
but did not tell them to use it as it was not what they asked for.

Furthermore, the CMS needs a major refactor based on it's performance for static
content generation and that is where we should focus our efforts rather than
debating the neccessity of 20 lines of code which is not exceptionally complex
or innefficient and has a rather simple API.

But as long as the website developers are happy to use selectattr I have nothing
more time say about this.

[Vasily Kuznetsov, 2018-04-17 18:12:10]

On 2018/04/17 16:20:09, Sebastian Noack wrote:
> On 2018/04/17 15:12:53, Vasily Kuznetsov wrote:
> > The current implementation of `get_pages_metadata` is a fairly independent
and
> > small piece of code in the CMS. I don't think getting rid of filtering there
> > (~20 lines of code that doesn't call anything else) would have any material
> > effect on the complexity of the CMS. If it also makes Julian and other
website
> > developers less happy, I would see it as a step in the wrong direction.
> 
> Well, 20 lines of code (though probably some more when accounting for tests)
> might not be a ridiculous amount of complexity, but compared to the little
> boilerplate this patch would add, still not really worth to maintain, if it
> wouldn't be for the plain purpose of avoiding this debate.
> 
> FWIW, one could also see the changes here as an improvement (as opposed to a
> compromise in favor of CMS developers). At least the selectattr() filter is a
> well-documented feature of a standard tool (i.e. jinja). So every developer
who
> knows the jinja template language, immediately understands what this template
> code does, or at very least everyone can figure it out quickly, without
digging
> into the CMS. Not to mention that it is more flexible too (i.e. you can filter
> by any attribute, in any kind of sequence in a bunch of different ways).
> 
> It's not about get_pages_metadata() being part of the CMS. We need some stable
> interface towards the websites, and a template function is straight-forward.
> Having an implementation of get_pages_metadata() somewhere else relying on
> internal implementation details of the CMS, will just cause an even larger
mess.
> It is more about not reinventing the wheel (and then insisting on making it
> square-shaped). Did you ever complain that you have to use grep and not have
> (more limited) output filtering built into every single command instead?

You make some good arguments. However, I can imagine some counterarguments too.
I guess I'll update my position to neutral, drop my proposal about moving the
function out (because stable API is good), and let Julian decide.

[juliandoucette, 2018-04-18 11:24:39]

CC Manvel

This is becoming a lot to read :D.

I'd like to get feedback from Ire and Manvel before moving forward.

FWIW: I think that we all agree that we should be capable of implementing
get_pages_metadata type filtering in website-defaults if we choose. And whatever
is necessary to feed selectattr would also be necessary for a website-defaults
get_pages_metadata.

Generally, I'm in favour of reducing the complexity (and verbosity) of our
templates as-much-as-possible. Frankly, I think Jinja2 templating can be
unnecessarily complex/unintuitive sometimes. And I'd like to phase them out in
favour of another format eventually.

[saroyanm, 2018-04-18 14:40:24]

On 2018/04/18 11:24:39, juliandoucette wrote:
> CC Manvel
> 
> This is becoming a lot to read :D.
> 
> I'd like to get feedback from Ire and Manvel before moving forward.
> 
> FWIW: I think that we all agree that we should be capable of implementing
> get_pages_metadata type filtering in website-defaults if we choose. And
whatever
> is necessary to feed selectattr would also be necessary for a website-defaults
> get_pages_metadata.
Considering the fact that website-defaults suppose to be used in the future in
all our websites, I think there shouldn't be much difference whether it's
implemented in CMS or in the  website-defaults,
  
> Generally, I'm in favour of reducing the complexity (and verbosity) of our
> templates as-much-as-possible. Frankly, I think Jinja2 templating can be
> unnecessarily complex/unintuitive sometimes. And I'd like to phase them out in
> favour of another format eventually.
You and Ire worked with the get_pages_metadata much more than I did, so I don't
have as much experience as you have, but anyway I agree, if the existing
implementation is something that's more intuitive/nicer to work with, then make
sense to stick to it. (Personal opinion): I like the fact that the current
implementation is compact, than the jinja2 filters.

[ire, 2018-04-19 09:48:15]

Hey everyone, sorry for the late response I was away the last 2 days. Here are
my thoughts:

I think that it isn’t ideal to replicate the selectattr() functionality in the
CMS in what is only a slightly simpler format. I agree with Sebastian’s point
about confusion because the Jinja methods are better documented than the CMS
methods, and speaking from my own experience it was a bit confusing knowing what
methods do what. 

I don’t, however, think the answer is to remove the filtering in
get_pages_metadata() because that doesn’t really help us as the website
developers. 

An ideal solution would be what Vasily suggested, for us to create a wrapper for
selectattr() as a global, but I don’t think that is possible as I tried and it
seems that neither get_pages_metadata() nor selectattr() are accessible from
files in the globals directory (Does anyone know a way around this?).

[Sebastian Noack, 2018-04-19 10:23:11]

On 2018/04/19 09:48:15, ire wrote:
> An ideal solution would be what Vasily suggested, for us to create a wrapper
for
> selectattr() as a global, but I don’t think that is possible as I tried and it
> seems that neither get_pages_metadata() nor selectattr() are accessible from
> files in the globals directory (Does anyone know a way around this?).

You can write such a wrapper as template macro:

  {% macro filter_pages() %}
    {%- set ns = namespace(pages=get_pages_metadata()) %}
    {%- for attr, value in kwargs.items() %}
      {%- set ns.pages = ns.pages|selectattr(attr, "equalto", value) %}
    {%- endfor %}
    {{- caller(ns.pages|list) }}
  {%- endmacro %}

Which then can be used like this:

  {% call(pages) filter_pages(template="article", popular="true") %}
    {% if pages %}
      ...
      {% for page in pages %}
        ...
      {% endfor %}
      ...
    {% endif %}
  {% endcall %}

[ire, 2018-04-19 10:58:46]

On 2018/04/19 10:23:11, Sebastian Noack wrote:
> On 2018/04/19 09:48:15, ire wrote:
> > An ideal solution would be what Vasily suggested, for us to create a wrapper
> for
> > selectattr() as a global, but I don’t think that is possible as I tried and
it
> > seems that neither get_pages_metadata() nor selectattr() are accessible from
> > files in the globals directory (Does anyone know a way around this?).
> 
> You can write such a wrapper as template macro:
> 
>   {% macro filter_pages() %}
>     {%- set ns = namespace(pages=get_pages_metadata()) %}
>     {%- for attr, value in kwargs.items() %}
>       {%- set ns.pages = ns.pages|selectattr(attr, "equalto", value) %}
>     {%- endfor %}
>     {{- caller(ns.pages|list) }}
>   {%- endmacro %}
> 
> Which then can be used like this:
> 
>   {% call(pages) filter_pages(template="article", popular="true") %}
>     {% if pages %}
>       ...
>       {% for page in pages %}
>         ...
>       {% endfor %}
>       ...
>     {% endif %}
>   {% endcall %}

Thanks Sebastian! I would be fine with a solution like this.

[Sebastian Noack, 2018-04-19 11:06:26]

On 2018/04/19 10:58:46, ire wrote:
> Thanks Sebastian! I would be fine with a solution like this.

Awesome! Only thing is, this requires jinja >=2.10? Just in case, does anybody
knows which version we have (can get) on the server?

[Vasily Kuznetsov, 2018-04-19 15:23:23]

On 2018/04/19 11:06:26, Sebastian Noack wrote:
> On 2018/04/19 10:58:46, ire wrote:
> > Thanks Sebastian! I would be fine with a solution like this.
> 
> Awesome! Only thing is, this requires jinja >=2.10? Just in case, does anybody
> knows which version we have (can get) on the server?

It seems that adblockplus.org has 2.8 right now:

vasily@adblockplus-org-1:~$ python
Python 2.7.9 (default, Jun 29 2016, 13:08:31)
[GCC 4.9.2] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import jinja2
>>> jinja2.__version__
'2.8'
>>>

I imagine it could be updated though. Also when we switch to using
gitlab/CI/whatever approach for building websites, it would be even easier to
upgrade Jinja to whatever is needed.

[juliandoucette, 2018-04-23 14:15:26]

On 2018/04/19 10:23:11, Sebastian Noack wrote:
> On 2018/04/19 09:48:15, ire wrote:
> > An ideal solution would be what Vasily suggested, for us to create a wrapper
> for
> > selectattr() as a global, but I don’t think that is possible as I tried and
it
> > seems that neither get_pages_metadata() nor selectattr() are accessible from
> > files in the globals directory (Does anyone know a way around this?).
> 
> You can write such a wrapper as template macro:
> 
>   {% macro filter_pages() %}
>     {%- set ns = namespace(pages=get_pages_metadata()) %}
>     {%- for attr, value in kwargs.items() %}
>       {%- set ns.pages = ns.pages|selectattr(attr, "equalto", value) %}
>     {%- endfor %}
>     {{- caller(ns.pages|list) }}
>   {%- endmacro %}
> 
> Which then can be used like this:
> 
>   {% call(pages) filter_pages(template="article", popular="true") %}
>     {% if pages %}
>       ...
>       {% for page in pages %}
>         ...
>       {% endfor %}
>       ...
>     {% endif %}
>   {% endcall %}

Can you enable us to use a filter instead of a macro?

e.g.

{% for page in pages | query(category="Example") %}

[juliandoucette, 2018-06-08 17:12:21]

On 2018/04/23 14:15:26, juliandoucette wrote:
> Can you enable us to use a filter instead of a macro?
> 
> e.g.
> 
> {% for page in pages | query(category="Example") %}

BUMP

[Vasily Kuznetsov, 2018-07-09 18:07:45]

On 2018/06/08 17:12:21, juliandoucette wrote:
> On 2018/04/23 14:15:26, juliandoucette wrote:
> > Can you enable us to use a filter instead of a macro?
> > 
> > e.g.
> > 
> > {% for page in pages | query(category="Example") %}
> 
> BUMP

Hi Julian.

I'm a bit unsure what we want to do here: upgrade Jinja and use selectattr() or
create a new, less verbose, filter in CMS or create same filter in
website-defaults. Perhaps we should create a ticket for the desired change and
then move things forward there.

Cheers,
Vasily

[Jon Sonesen, 2018-07-09 18:28:00]

On 2018/07/09 18:07:45, Vasily Kuznetsov wrote:
> On 2018/06/08 17:12:21, juliandoucette wrote:
> > On 2018/04/23 14:15:26, juliandoucette wrote:
> > > Can you enable us to use a filter instead of a macro?
> > > 
> > > e.g.
> > > 
> > > {% for page in pages | query(category="Example") %}
> > 
> > BUMP
> 
> Hi Julian.
> 
> I'm a bit unsure what we want to do here: upgrade Jinja and use selectattr()
or
> create a new, less verbose, filter in CMS or create same filter in
> website-defaults. Perhaps we should create a ticket for the desired change and
> then move things forward there.
> 
> Cheers,
> Vasily